From cb5f9df0c8932fe0db9f783ede7893b7efcadcdb Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 13 Sep 2016 17:04:57 +0200 Subject: [PATCH] [EnhanceTextSelection] Make `expandTextDivs` more efficient by updating all styles at once instead of piecewise I intended to provide proper benchmarking results here, as outlined in https://github.com/mozilla/pdf.js/wiki/Benchmarking-your-changes, but after wasting a couple of hours over the weekend getting weird results I gave up. It appears that there's a lot of, i.e. way too much, variance between subsequent runs of `text` tests for the results to be meaningful. (Previously I've only benchmarked `eq` tests, so I don't know if the `text` tests has never worked well or if it's a newer problem. For reference, please see the results of back-to-back benchmark runs on the current `master` with a *very* simple manifest file: [link here].) Instead I used `console.time/timeEnd` in `appendText` and `expandTextDivs` to be able to compare the performance with/without the patch. The entire viewer was (skip-cache) reloaded between measurements, and the result are available here: [link here]. Given the troubles I've had with benchmarking, I've not yet computed any statistics on the results (e.g. mean, variance, confidence intervals, and so on). However, just by looking at the data I think it's safe to say that this patch first of all doesn't seem to regress the current performance. Secondly it certainly looks *very* likely that this patch actually improves the performance, especially for the one-glyph-per-text-div case (cf. issue 7224). Re: issue 7584. --- src/display/text_layer.js | 48 +++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/src/display/text_layer.js b/src/display/text_layer.js index a1e82e0b2..893c6ea7d 100644 --- a/src/display/text_layer.js +++ b/src/display/text_layer.js @@ -59,14 +59,20 @@ var renderTextLayer = (function renderTextLayerClosure() { return !NonWhitespaceRegexp.test(str); } + // Text layers may contain many thousand div's, and using `styleBuf` avoids + // creating many intermediate strings when building their 'style' properties. + var styleBuf = ['left: ', 0, 'px; top: ', 0, 'px; font-size: ', 0, + 'px; font-family: ', '', ';']; + function appendText(task, geom, styles) { // Initialize all used properties to keep the caches monomorphic. var textDiv = document.createElement('div'); var textDivProperties = { + style: null, angle: 0, canvasWidth: 0, isWhitespace: false, - originalTransform: '', + originalTransform: null, paddingBottom: 0, paddingLeft: 0, paddingRight: 0, @@ -104,10 +110,12 @@ var renderTextLayer = (function renderTextLayerClosure() { left = tx[4] + (fontAscent * Math.sin(angle)); top = tx[5] - (fontAscent * Math.cos(angle)); } - textDiv.style.left = left + 'px'; - textDiv.style.top = top + 'px'; - textDiv.style.fontSize = fontHeight + 'px'; - textDiv.style.fontFamily = style.fontFamily; + styleBuf[1] = left; + styleBuf[3] = top; + styleBuf[5] = fontHeight; + styleBuf[7] = style.fontFamily; + textDivProperties.style = styleBuf.join(''); + textDiv.setAttribute('style', textDivProperties.style); textDiv.textContent = geom.str; // |fontName| is only used by the Font Inspector. This test will succeed @@ -517,7 +525,6 @@ var renderTextLayer = (function renderTextLayerClosure() { this._renderTimer = null; this._bounds = []; this._enhanceTextSelection = !!enhanceTextSelection; - this._expanded = false; } TextLayerRenderTask.prototype = { get promise() { @@ -555,18 +562,20 @@ var renderTextLayer = (function renderTextLayerClosure() { if (!this._enhanceTextSelection || !this._renderingDone) { return; } - if (!this._expanded) { + if (this._bounds !== null) { expand(this); - this._expanded = true; - this._bounds.length = 0; + this._bounds = null; } for (var i = 0, ii = this._textDivs.length; i < ii; i++) { var div = this._textDivs[i]; var divProperties = this._textDivProperties.get(div); + if (divProperties.isWhitespace) { + continue; + } if (expandDivs) { - var transform = ''; + var transform = '', padding = ''; if (divProperties.scale !== 1) { transform = 'scaleX(' + divProperties.scale + ')'; @@ -575,21 +584,26 @@ var renderTextLayer = (function renderTextLayerClosure() { transform = 'rotate(' + divProperties.angle + 'deg) ' + transform; } if (divProperties.paddingLeft !== 0) { - div.style.paddingLeft = - (divProperties.paddingLeft / divProperties.scale) + 'px'; + padding += ' padding-left: ' + + (divProperties.paddingLeft / divProperties.scale) + 'px;'; transform += ' translateX(' + (-divProperties.paddingLeft / divProperties.scale) + 'px)'; } if (divProperties.paddingTop !== 0) { - div.style.paddingTop = divProperties.paddingTop + 'px'; + padding += ' padding-top: ' + divProperties.paddingTop + 'px;'; transform += ' translateY(' + (-divProperties.paddingTop) + 'px)'; } if (divProperties.paddingRight !== 0) { - div.style.paddingRight = - (divProperties.paddingRight / divProperties.scale) + 'px'; + padding += ' padding-right: ' + + (divProperties.paddingRight / divProperties.scale) + 'px;'; } if (divProperties.paddingBottom !== 0) { - div.style.paddingBottom = divProperties.paddingBottom + 'px'; + padding += ' padding-bottom: ' + + divProperties.paddingBottom + 'px;'; + } + + if (padding !== '') { + div.setAttribute('style', divProperties.style + padding); } if (transform !== '') { CustomStyle.setProp('transform', div, transform); @@ -597,7 +611,7 @@ var renderTextLayer = (function renderTextLayerClosure() { } else { div.style.padding = 0; CustomStyle.setProp('transform', div, - divProperties.originalTransform); + divProperties.originalTransform || ''); } } },