From 106b239c5d9319c454bbf8db7ad75215a9f887cc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 28 Aug 2019 16:08:06 +0200 Subject: [PATCH 1/2] [TextLayer] Avoid unnecessary font updates in `_layoutText` (PR 11097 follow-up) *This should obviously have been done in PR 11097, but for some reason I completely overlooked it; sorry about that.* There's no good reason to update the font unless you're actually going to measure the width of the textContent. This can reduce unnecessary font switching a fair bit, even for documents which are somewhat simple/short (in e.g. the `tracemonkey.pdf` file this cuts the amount of font switches almost in half). --- src/display/text_layer.js | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/display/text_layer.js b/src/display/text_layer.js index 6027ffa82..cdac05ad5 100644 --- a/src/display/text_layer.js +++ b/src/display/text_layer.js @@ -525,26 +525,25 @@ var renderTextLayer = (function renderTextLayerClosure() { }, _layoutText(textDiv) { - let textLayerFrag = this._container; - - let textDivProperties = this._textDivProperties.get(textDiv); + const textDivProperties = this._textDivProperties.get(textDiv); if (textDivProperties.isWhitespace) { return; } - const { fontSize, fontFamily, } = textDiv.style; - - // Only build font string and set to context if different from last. - if (fontSize !== this._layoutTextLastFontSize || - fontFamily !== this._layoutTextLastFontFamily) { - this._layoutTextCtx.font = `${fontSize} ${fontFamily}`; - this._layoutTextLastFontSize = fontSize; - this._layoutTextLastFontFamily = fontFamily; - } let transform = ''; if (textDivProperties.canvasWidth !== 0) { + const { fontSize, fontFamily, } = textDiv.style; + + // Only build font string and set to context if different from last. + if (fontSize !== this._layoutTextLastFontSize || + fontFamily !== this._layoutTextLastFontFamily) { + this._layoutTextCtx.font = `${fontSize} ${fontFamily}`; + this._layoutTextLastFontSize = fontSize; + this._layoutTextLastFontFamily = fontFamily; + } // Only measure the width for multi-char text divs, see `appendText`. const { width, } = this._layoutTextCtx.measureText(textDiv.textContent); + if (width > 0) { textDivProperties.scale = textDivProperties.canvasWidth / width; transform = `scaleX(${textDivProperties.scale})`; @@ -560,7 +559,7 @@ var renderTextLayer = (function renderTextLayerClosure() { textDiv.style.transform = transform; } this._textDivProperties.set(textDiv, textDivProperties); - textLayerFrag.appendChild(textDiv); + this._container.appendChild(textDiv); }, _render: function TextLayer_render(timeout) { From 667e548e5febc108bd824819e0ca135cb0082147 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 28 Aug 2019 16:17:30 +0200 Subject: [PATCH 2/2] [TextLayer] Remove `setAttribute` usage in `appendText` (issue 8066) One of the motivations for using `setAttribute` in the first place was to support more efficient DOM updates in the `expandTextDivs` method, since performance of the `enhanceTextSelection` mode can be somewhat bad when there's a lot of `textDivs` on the page. With recent `TextLayer` changes/optimizations it's no longer necessary to store a complete `style`-string for every `textDiv`, and we can thus re-visit the `setAttribute` usage. Note that with the current code, in `appendText`, there's only *one* string per `textDiv` which avoids a bunch of temporary strings. While the changes in this patch means that there's now *three* strings per `textDiv` instead, the total length of these strings are now quite a bit shorter (42 characters to be exact). --- src/display/text_layer.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/display/text_layer.js b/src/display/text_layer.js index cdac05ad5..1bd06bfd5 100644 --- a/src/display/text_layer.js +++ b/src/display/text_layer.js @@ -48,11 +48,6 @@ var renderTextLayer = (function renderTextLayerClosure() { return !NonWhitespaceRegexp.test(str); } - // Text layers may contain many thousands of divs, 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('span'); @@ -97,11 +92,12 @@ var renderTextLayer = (function renderTextLayerClosure() { left = tx[4] + (fontAscent * Math.sin(angle)); top = tx[5] - (fontAscent * Math.cos(angle)); } - styleBuf[1] = left; - styleBuf[3] = top; - styleBuf[5] = fontHeight; - styleBuf[7] = style.fontFamily; - textDiv.setAttribute('style', styleBuf.join('')); + // Setting the style properties individually, rather than all at once, + // should be OK since the `textDiv` isn't appended to the document yet. + textDiv.style.left = `${left}px`; + textDiv.style.top = `${top}px`; + textDiv.style.fontSize = `${fontHeight}px`; + textDiv.style.fontFamily = style.fontFamily; textDiv.textContent = geom.str; // `fontName` is only used by the FontInspector, and we only use `dataset`