402802 - Reduce browser.display.auto_quality_min_font_size

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch

browser.display.auto_quality_min_font_size was set to 60 (px) when I checked in the patch that created it. This means by default (text-rendering:auto) we use kerning and ligatures for font sizes >= 60px. I propose we reduce this so that it will actually kick in for some reasonable Web pages. Of course we don't want it to kick in too often or perf will hurt. Hence I propose 20.

The patch is trivial. There is of course a Tp risk here, so we'd need to watch tboxes carefully when making the change.

Comment 1

[Stuart Parmenter]

Is it going to be too confusing to web authors to have their text rendering different at different sizes?  With 60, you won't see it too often... With 20 I suspect you'd see it reasonably often.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Text already does render differently at different sizes, for various reasons (including things like antialiasing and hinting turning on and off). This is adding a little more variation. It may be a *little* confusing but it won't break things IMHO.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Some users have a default, or even min, font size pref greater than 20px.  Perhaps the pref should be expressed as a multiple of the default font size?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How many users? If it's only a small number, then the perf hit they see shouldn't concern us.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Not going to block on this, but we need to figure out the right value here -- if we check this in and there's no talos impact, then let's take it, otherwise we need to reevaluate.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure. I wasn't planning on doing performance work to make this patch acceptable. It either flies or it doesn't. BTW you still need to review it :-)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I landed this. No obvious performance impact, although I've only had one cycle of Talos Tp on one box (Talos is slow!!!). I'll check again tomorrow.

Comment 8

[Stuart Parmenter]

this caused a pretty big (8k?) allocation regression on linux and prolly windows-- as I said earlier in this bug I don't think 20 is the right size (guess I should have r-'d it.)  should probably back this out until we can determine the right size.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Well, more work will be happening that wasn't happening before, so more allocations doesn't seem unreasonable -- there being no performance impact is more interesting, because this should be a pretty significant increase in text quality.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see a significant change on Windows allocations. Before the patch it was mostly 297-298K with occasional 290K, after the patch is the same.

I'm actually very pleased that Tp did not budge. I think 8K more allocations on Linux, with no significant max-heap increase, is a reasonable price to pay for this functionality. We could file a followup bug to diagnose the 8K allocations (if someone tells me what the workload is on that test) and see if there's a way to reduce them, but even if there isn't, I think we should keep this.