From 3e391aaed99a59cc82a33a311cb50b80df71fd53 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 18 Oct 2022 13:43:42 +0200 Subject: [PATCH 1/2] Remove the `Glyph.matchesForCache` method (PR 13494 follow-up) This method, and its class, was originally added in PR 4453 to reduce memory usage when parsing text. Then PR 13494 extended the `Glyph`-representation slightly to also include the `charCode`, which made the `matchesForCache` method *effectively* redundant since most properties on a `Glyph`-instance indirectly depends on that one. The only exception is potentially `isSpace` in multi-byte strings. Also, something that I noticed when testing this code: The `matchesForCache` method never worked correctly for `Glyph`s containing `accent`-data, since Objects are passed by reference in JavaScript. For affected fonts, of which there's only a handful of examples in our test-suite, we'd fail to find an already existing `Glyph` because of this. --- src/core/fonts.js | 41 +++-------------------------------------- 1 file changed, 3 insertions(+), 38 deletions(-) diff --git a/src/core/fonts.js b/src/core/fonts.js index 4c4827587..09eba857f 100644 --- a/src/core/fonts.js +++ b/src/core/fonts.js @@ -218,30 +218,6 @@ class Glyph { this.isZeroWidthDiacritic = category.isZeroWidthDiacritic; this.isInvisibleFormatMark = category.isInvisibleFormatMark; } - - matchesForCache( - originalCharCode, - fontChar, - unicode, - accent, - width, - vmetric, - operatorListId, - isSpace, - isInFont - ) { - return ( - this.originalCharCode === originalCharCode && - this.fontChar === fontChar && - this.unicode === unicode && - this.accent === accent && - this.width === width && - this.vmetric === vmetric && - this.operatorListId === operatorListId && - this.isSpace === isSpace && - this.isInFont === isInFont - ); - } } function int16(b0, b1) { @@ -3314,20 +3290,9 @@ class Font { } let glyph = this._glyphCache[charcode]; - if ( - !glyph || - !glyph.matchesForCache( - charcode, - fontChar, - unicode, - accent, - width, - vmetric, - operatorListId, - isSpace, - isInFont - ) - ) { + // All `Glyph`-properties, except `isSpace` in multi-byte strings, + // depend indirectly on the `charcode`. + if (!glyph || glyph.isSpace !== isSpace) { glyph = new Glyph( charcode, fontChar, From fd35cda8bc3370e1bd412b4ee7d7c5900c31e78c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 18 Oct 2022 17:20:25 +0200 Subject: [PATCH 2/2] Re-factor the glyph-cache lookup in the `Font._charToGlyph` method With the changes in the previous patch we can move the glyph-cache lookup to the top of the method and thus avoid a bunch of, in *almost* every case, completely unnecessary re-parsing for every `charCode`. --- src/core/fonts.js | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/core/fonts.js b/src/core/fonts.js index 09eba857f..b42486506 100644 --- a/src/core/fonts.js +++ b/src/core/fonts.js @@ -3225,6 +3225,12 @@ class Font { * @private */ _charToGlyph(charcode, isSpace = false) { + let glyph = this._glyphCache[charcode]; + // All `Glyph`-properties, except `isSpace` in multi-byte strings, + // depend indirectly on the `charcode`. + if (glyph && glyph.isSpace === isSpace) { + return glyph; + } let fontCharCode, width, operatorListId; let widthCode = charcode; @@ -3289,24 +3295,18 @@ class Font { } } - let glyph = this._glyphCache[charcode]; - // All `Glyph`-properties, except `isSpace` in multi-byte strings, - // depend indirectly on the `charcode`. - if (!glyph || glyph.isSpace !== isSpace) { - glyph = new Glyph( - charcode, - fontChar, - unicode, - accent, - width, - vmetric, - operatorListId, - isSpace, - isInFont - ); - this._glyphCache[charcode] = glyph; - } - return glyph; + glyph = new Glyph( + charcode, + fontChar, + unicode, + accent, + width, + vmetric, + operatorListId, + isSpace, + isInFont + ); + return (this._glyphCache[charcode] = glyph); } charsToGlyphs(chars) {