Closed Bug 1443314 Opened 2 years ago Closed 2 years ago

Consider removing the svg.path-caching.enabled pref

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: froydnj, Assigned: longsonr)

Details

Attachments

(1 file)

svg.path-caching.enabled was flipped to true in bug 932762.

svg.display-lists.{hit-testing,painting}.enabled were set to true in bug 776054.

The former is 3.5 years old, the latter about 5.5.  I suspect that we've rarely tested with those prefs set to false over the past several years.

Could we declare victory and just remove those prefs (and their associated caches) entirely?  dholbert, do you know who would be the best person to make that call?
Flags: needinfo?(dholbert)
Indeed, we can probably remove these prefs. jwatt, sound good to you?
Flags: needinfo?(dholbert) → needinfo?(jwatt)
Thanks for looking for dead prefs. Is there much of a reason to remove them other than just to clean things up? Do we have so many content prefs to pass over early IPC that it's causing issues, for example?

See bug 829802 for the display list prefs.

Regarding svg.path-caching.enabled, the very limited profiling that I did after path caching was added indicated that it wasn't much of a win, although Bas was surprised about that and wanted more testing. That never happened since we were very busy around that time. Ideally we should do that though, and remove path caching itself if we don't see it helping since otherwise it just needlessly increases our memory use.

That said, we would do that with a hacked build, so I'm fine with removing this pref.
Flags: needinfo?(jwatt)
Summary: consider removing SVG path caching- and display list-related prefs → Consider removing the svg.path-caching.enabled pref
(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #2)
> Thanks for looking for dead prefs. Is there much of a reason to remove them
> other than just to clean things up? Do we have so many content prefs to pass
> over early IPC that it's causing issues, for example?

This came out of examining nsLayoutStatics as part of looking at our initialization process and trying to understand what everything was doing.  A lot of code called from nsLayoutStatics seems to be in the business of setting up pref caches and the like.

I don't think there's much reason to remove them other than cleanup, but cleaning up options that are always on and aren't likely to be flipped off is a nice thing to do in general.
"just to clean things up" is a perfectly valid reason to remove prefs. We have over 3000 prefs, many of which are likely unnecessary and should be removed.
Attached patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #8959949 - Flags: review?(jwatt)
Attachment #8959949 - Flags: review?(jwatt) → review+
(In reply to Nicholas Nethercote [:njn] from comment #4)
> "just to clean things up" is a perfectly valid reason to remove prefs.

Absolutely. I wanted to know if there were additional reasons I should be aware of.
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae5b14ff800
remove the svg.path-caching.enabled pref r=jwatt
> Absolutely. I wanted to know if there were additional reasons I should be
> aware of.

This Google Doc has some info about that:
https://docs.google.com/document/d/1V5Wc9bXwfgMG2JOsswvDPVwZl_xiaSBxwJXf3fiEaU8/edit#
https://hg.mozilla.org/mozilla-central/rev/4ae5b14ff800
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.