Speed up font-width computation in most cases #1390

paulmelnikow · 2017-12-24T02:00:18Z

This takes a naive approach to font-width computation, the most compute-intensive part of rendering badges.

Add the widths of the individual characters.
- These widths are measured on startup using PDFKit.
For each character pair, add a kerning adjustment
- The difference between the width of each character pair, and the sum of the characters' separate widths.
- These are computed for each character pair on startup using PDFKit.
For a string with characters outside the printable ASCII character set, fall back to PDFKit.

This branch averaged 0.049 ms in makeBadge, compared to 0.182 ms on master, a speedup of 73%. That was on a test of 10,000 consecutive requests (using the method in #1379).

The speedup applies to badges containing exclusively printable ASCII characters. It wouldn't be as dramatic on non-ASCII text. Though, we could add some frequently used non-ASCII characters to the cached set.

shields-ci · 2017-12-24T02:01:13Z

	Warnings
⚠️	This PR modified the server but none of the service tests. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS

paulmelnikow · 2017-12-24T04:13:45Z

I can replicate this failure locally. It must have crept in as I was cleaning up the code.

I think the problem is due to global state and test ordering. They used to pass locally regardless of whether I chose Verdana or DejaVu Sans. Now they pass locally only with Verdana.

espadrine

Thanks a lot! This is solid work!

I can confirm that I replicate your findings related to the benchmark with a modification of this patch (going from 0.197ms to 0.048ms).

espadrine · 2017-12-24T13:11:12Z

lib/measure-text.js

+module.exports = {
+  PDFKitTextMeasurer,
+  QuickTextMeasurer,
+  // measure: defaultMeasurer.measure.bind(defaultMeasurer),


I liked having the measure function as the default export.

Also, I see no benefit to using promises for server initialization procedures.
Could we simply have initialization be synchronous?

(As a result, the distinction between new TextMeasurer() and TextMeasurer.create() is not that useful.)

I liked having the measure function as the default export.

Will try to restore that. Was in the middle of cleaning up a gnarly module state bug, though if the initialization can all be done synchronously that will help a lot. Building the measurer is a little slow, so it would probably be good to avoid doing it when it's not necessary. Will think about a way to do that.

Could we simply have initialization be synchronous?

Totally! I completely missed that. loadFont was using a callback before and I robotically ported it to promises, not realizing all the work was synchronous. That's great. It'll simplify this a lot.

I ended up removing this, because the quick measurer takes a while to generate, and it was slowing down the CLI and the CLI tests. Plus it makes running exclusive tests longer, as the cache needs to be built even if it's not used by the tests being run.

espadrine · 2017-12-24T13:16:06Z

lib/make-badge.js

@@ -152,6 +152,5 @@ function makeBadge (data) {
 }

 module.exports = makeBadge;
-module.exports.loadFont = measureTextWidth.loadFont;


Was that umused?

Yea, I think so. It might have been part of the gh-badges library usage. If it is I'll make sure the docs are updated as part of #1388.

espadrine · 2017-12-24T13:17:52Z

lib/measure-text.js

  }
 }

-loadFont(path.join(__dirname, '..', 'Verdana.ttf'), function (err) {
-  if (err && process.env.FALLBACK_FONT_PATH) {
-    loadFont(process.env.FALLBACK_FONT_PATH);


How do you plan on keeping supporting FALLBACK_FONT_PATH and using FONT_PATH?

If I am reading the patch correctly, you currently no longer use any of them.

Yea, sorry, that bit was WIP. I'll get it fixed up.

espadrine · 2017-12-24T13:34:22Z

lib/measure-text.js

-    loadFont(process.env.FALLBACK_FONT_PATH);
+class QuickTextMeasurer {
+  constructor(baseMeasurer) {
+    Object.assign(this, { baseMeasurer });


Do we benefit from the generality this is meant to create?
Shouldn't we directly use PDFKitTextMeasurer?

Injecting it makes testing a little easier, because I can place the spy on the measurer instance. Caching code is notoriously difficult to test so I feel it's important to test that the cache object doesn't call through to the base object when it's not being used. I'll take a look though; maybe that is easily changed now. When this is all synchronous it'll be way simpler.

This is working just fine by mocking the method on the prototype instead!

paulmelnikow · 2017-12-25T17:31:41Z

Thanks for the review! Will pick this up today and respond to your comments.

This needs cleanup…

This reverts commit 267effd.

paulmelnikow · 2017-12-26T04:34:20Z

I re-ran the benchmark on the last commit and the 73% is holding up. I got 0.041 vs 0.144 in master.

This simplifies and further optimizes text-width computation by computing the entire width table in advance, and serializing it in the style of QuickTextMeasurer (#1390). This entirely removes the need for PDFKit at runtime. This has the advantage of fixing #1305 – more generally: producing the same result everywhere – without having to deploy a copy of Verdana. The lifting is delegated to these three libraries, which are housed in a monorepo: https://github.com/metabolize/anafanafo I'd be happy to move it into the badges org if folks want to collaborate on maintaining them. QuickTextMeasurer took kerning pairs into account, whereas this implementation does not. I was thinking kerning would be a necessary refinement, though this seems to work well enough. I dropped in a binary-search package to traverse the data structure, in part to conserve space. This causes a moderate performance regression, though there is ample room for improving on that: #2311 (comment)

Use naive approach to compute font width

d7a2c4b

paulmelnikow added core Server, BaseService, GitHub auth, Shared helpers performance-improvement Related to performance or throughput of the badge servers labels Dec 24, 2017

paulmelnikow mentioned this pull request Dec 24, 2017

Performance optimization of text-width computation #1379

Closed

See if this gets test passing

c3ba10f

espadrine reviewed Dec 25, 2017

View reviewed changes

paulmelnikow changed the title ~~Speed up font-width computation in most cases~~ [WIP] Speed up font-width computation in most cases Dec 25, 2017

paulmelnikow and others added 19 commits December 25, 2017 12:41

More cleanup to init pattern

5249f18

Fix font width perf

4f22511

Tests running on DejaVu and Verdana

1c163a7

More cleanup

877ee7e

First pass to inject measurer / makeBadge

6ae424b

This needs cleanup…

Speed up the CLI tests a bit

e762c0d

Try to fix CI build

2122c4f

Clean up a bunch of duplication

9667a86

More cleanup

ad3d7c2

Rename

6c788d6

Reset package-lock

267effd

Revert "Reset package-lock"

0f5efdd

This reverts commit 267effd.

Try that again

90bc1bb

Check in profiling script and put it behind a flag

92710a9

Rename

f8731e1

Fix import

0518ebc

Fail with a better error

00f4dfc

Improve error message

aab73cd

Fix syntax error

e3feab2

paulmelnikow changed the title ~~[WIP] Speed up font-width computation in most cases~~ Speed up font-width computation in most cases Dec 26, 2017

paulmelnikow merged commit cc9a6db into badges:master Dec 27, 2017

paulmelnikow deleted the font-width-perf branch December 27, 2017 04:57

paulmelnikow mentioned this pull request Jan 3, 2018

Release gh-badges to npm #1388

Closed

paulmelnikow mentioned this pull request Nov 14, 2018

Precompute text width using a lookup table #2311

Merged

Woodpile37 mentioned this pull request Aug 26, 2023

[Snyk] Upgrade danger from 11.1.1 to 11.2.8 Woodpile37/shields#54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up font-width computation in most cases #1390

Speed up font-width computation in most cases #1390

paulmelnikow commented Dec 24, 2017 •

edited

Loading

shields-ci commented Dec 24, 2017 •

edited

Loading

paulmelnikow commented Dec 24, 2017

espadrine left a comment

espadrine Dec 24, 2017

paulmelnikow Dec 25, 2017

paulmelnikow Dec 26, 2017

espadrine Dec 24, 2017

paulmelnikow Dec 26, 2017

espadrine Dec 24, 2017

paulmelnikow Dec 25, 2017

paulmelnikow Dec 26, 2017

espadrine Dec 24, 2017

paulmelnikow Dec 25, 2017

paulmelnikow Dec 26, 2017

paulmelnikow commented Dec 25, 2017

paulmelnikow commented Dec 26, 2017

Speed up font-width computation in most cases #1390

Speed up font-width computation in most cases #1390

Conversation

paulmelnikow commented Dec 24, 2017 • edited Loading

shields-ci commented Dec 24, 2017 • edited Loading

paulmelnikow commented Dec 24, 2017

espadrine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Dec 25, 2017

paulmelnikow commented Dec 26, 2017

paulmelnikow commented Dec 24, 2017 •

edited

Loading

shields-ci commented Dec 24, 2017 •

edited

Loading