1507506 - Use a more compact representation for the list of bad-underline font families

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement, P5)

Description

[Jonathan Kew [:jfkthame]]

We maintain a list of font families (various CJK fonts) where the underline-offset field is known to be bad, and therefore we want to ignore it for layout purposes and place the underline at the bottom of the em-square instead.

Currently, this list is stored in a hashtable in gfxPlatformFontList, and then as each gfxFontFamily record is created during platform font list setup, we check the family against the hashtable and set a flag if it's "bad".

However, we don't really need to be using a hashtable (with its associated memory overhead) here; a sorted array that we can just binary-search will be near enough equally performant for a small set of names, and has a smaller footprint.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Use a sorted array rather than a hashtable for the short list of bad-underline font families

Just a tiny memory optimization (worth a few hundred bytes) that I happened to notice while working through font-list code.

Comment 2

[Jonathan Watt [:jwatt]]

Comment on attachment 9025378 [details] [diff] [review]
Use a sorted array rather than a hashtable for the short list of bad-underline font families

Review of attachment 9025378 [details] [diff] [review]:
-----------------------------------------------------------------

If we had benchmarks to know where the crossover point is in terms of perf, I'd suggest an assert in LoadBadUnderlineList that the list is smaller than than value. But I suspect you have better things to do with your time. ;)

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 9025378 [details] [diff] [review]
Use a sorted array rather than a hashtable for the short list of bad-underline font families

Not sure what happened to my r+...

Comment 4

[Jonathan Kew [:jfkthame]]

Yeah, I'm not sure where the crossover would lie. Some brief instrumentation I did here suggests that the hashtable may be just a hair faster on average (by a few nanoseconds per lookup), though there's a lot of variability. And OTOH the hashtable takes a microsecond or two longer to initialize in LoadBadUnderlineList than the array, so there's that to consider.

How many lookups happen is dependent on the number of fonts installed on the system, so that would also affect the calculation of which option is more performant; for a user with only a few fonts (or with an anti-fingerprinting whitelist in effect) we do a lot fewer lookups and so the cost of building the hashtable might exceed the benefit of using it.

(As it is, with the bad-underline list being only a couple dozen items, and the lookup being done only once per font family, we could even just do a linear search of an unsorted array and I suspect it'd be hard to spot the difference!)

Just FTR, in my local build on macOS (where I have somewhere over 1000 font families installed), the total amount of time spent looking them up in this list (either before or after this patch) was generally in the range of 100-120µs.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03118fe1d6cc
Use a sorted array rather than a hashtable for the short list of bad-underline font families. r=jwatt

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/03118fe1d6cc