Closed Bug 1473634 Opened 2 years ago Closed Last year

Support matching multiple preferences per preference callback

Categories

(Core :: Preferences: Backend, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:10k])

Attachments

(3 files)

We currently have several places in the codebase which add a large number of callbacks with the same callback function and closure, but different preferences. The worst offenders are nsPresContext::PrefChangedCallback and MarkComputedStyleMapDirty.

PresContext only adds 640 bytes worth of callbacks per instance, but we create a lot of instances. In the parent process, it adds up to about 15K in a fresh session.

nsDOMComputedStyle only adds one set of MarkComputedStyleMapDirty callbacks per process, but it adds *a lot* of them. That adds up to about 5K per process.


For reference, in a fresh session, the most prevalent observers in the parent and content processes are:

├──22,040 B (00.01%) ++ mozilla::BoolVarChanged(char const*, void*)
├──16,560 B (00.01%) ++ nsPrefBranch::NotifyObserver(char const*, void*)
├──15,360 B (00.01%) ++ nsPresContext::PrefChangedCallback(char const*, void*)
├──11,120 B (00.01%) ++ OnGfxPrefChanged(char const*, void*)
├───5,320 B (00.00%) ── MarkComputedStyleMapDirty(char const*, void*)/150a5d0eb338 [133]
├───5,040 B (00.00%) ++ mozilla::IntVarChanged(char const*, void*)
├───3,040 B (00.00%) ++ mozilla::UintVarChanged(char const*, void*)
├───1,880 B (00.00%) ++ mozilla::FloatVarChanged(char const*, void*)
├───1,000 B (00.00%) ++ mozilla::AtomicBoolVarChanged<(mozilla::MemoryOrdering)0>(char const*, void*)
├─────960 B (00.00%) ++ mozilla::AtomicBoolVarChanged<(mozilla::MemoryOrdering)2>(char const*, void*)
├─────440 B (00.00%) ++ SetMemoryPrefChangedCallbackInt(char const*, void*)
├─────400 B (00.00%) ── mozilla::CubebUtils::PrefChanged(char const*, void*)/0 [10]
├─────240 B (00.00%) ++ 0x150a57bb9850
...

├──18,200 B (00.08%) ++ mozilla::BoolVarChanged(char const*, void*)
├───5,320 B (00.02%) ── MarkComputedStyleMapDirty(char const*, void*)/149361df3338 [133]
├───4,720 B (00.02%) ++ mozilla::IntVarChanged(char const*, void*)
├───3,240 B (00.02%) ++ nsPrefBranch::NotifyObserver(char const*, void*)
├───2,080 B (00.01%) ++ mozilla::UintVarChanged(char const*, void*)
├───1,880 B (00.01%) ++ mozilla::FloatVarChanged(char const*, void*)
├───1,000 B (00.00%) ++ mozilla::AtomicBoolVarChanged<(mozilla::MemoryOrdering)0>(char const*, void*)
├─────960 B (00.00%) ++ mozilla::AtomicBoolVarChanged<(mozilla::MemoryOrdering)2>(char const*, void*)
├─────640 B (00.00%) ── nsPresContext::PrefChangedCallback(char const*, void*)/14935855f800 [16]
├─────440 B (00.00%) ++ SetMemoryPrefChangedCallbackInt(char const*, void*)
├─────400 B (00.00%) ── mozilla::CubebUtils::PrefChanged(char const*, void*)/0 [10]
├─────240 B (00.00%) ── (anonymous namespace)::PrefStore::UpdateStringPrefs(char const*, void*)/1493587a7510 [6]
├─────200 B (00.00%) ++ mozilla::AtomicIntVarChanged<(mozilla::MemoryOrdering)0>(char const*, void*)
├─────160 B (00.00%) ++ mozilla::AtomicUintVarChanged<(mozilla::MemoryOrdering)0>(char const*, void*)
├─────120 B (00.00%) ++ SetMemoryPrefChangedCallbackBool(char const*, void*)
...
Comment on attachment 8992124 [details]
Bug 1473634: Part 3 - Update nsDOMComputedStyle to use RegisterCallbacks.

https://reviewboard.mozilla.org/r/257004/#review263860

::: layout/style/nsComputedDOMStyle.cpp:5512
(Diff revision 1)
>    for (const auto* p = nsCSSProps::kPropertyPrefTable;
>         p->mPropID != eCSSProperty_UNKNOWN; p++) {
> -    nsCString name;
> -    name.AssignLiteral(p->mPref, strlen(p->mPref));
> -    Preferences::RegisterCallback(MarkComputedStyleMapDirty, name, data);
> +    // Many properties are controlled by the same preference, so de-duplicate
> +    // them before adding observers.
> +    if (!prefs.ContainsSorted(p->mPref)) {
> +      prefs.InsertElementSorted(p->mPref);

So this is sorting the prefs by pointer value, not string, right?  I guess this works to dedup because they're all string literals so multiple instances of the same string already got dedupped by the compiler and linker?

There's another option here: decide on a pref name prefix that all prefs that matter here should have, assert that all the prefs in nsCSSProps::kPropertyPrefTable start with that prefix, and just register a single observer for that prefix.  Not sure how that compares to what we have here tradeoff-wise, and would involve some changes to other code to make the pref names all same-prefix, of course.

::: layout/style/nsComputedDOMStyle.cpp:5528
(Diff revision 1)
>  }
>  
>  /* static */ void
>  nsComputedDOMStyle::UnregisterPrefChangeCallbacks()
>  {
> -  ComputedStyleMap* data = GetComputedStyleMap();
> +  MOZ_ASSERT(gCallbackPrefs);

This assert would fail if nsLayoutStatics::Initialize failed out before it got to calling nsLayoutUtils::Initialize().  Probably better to just null-check and return as needed here.
Attachment #8992124 - Flags: review?(bzbarsky) → review+
Comment on attachment 8992124 [details]
Bug 1473634: Part 3 - Update nsDOMComputedStyle to use RegisterCallbacks.

https://reviewboard.mozilla.org/r/257004/#review263860

> So this is sorting the prefs by pointer value, not string, right?  I guess this works to dedup because they're all string literals so multiple instances of the same string already got dedupped by the compiler and linker?
> 
> There's another option here: decide on a pref name prefix that all prefs that matter here should have, assert that all the prefs in nsCSSProps::kPropertyPrefTable start with that prefix, and just register a single observer for that prefix.  Not sure how that compares to what we have here tradeoff-wise, and would involve some changes to other code to make the pref names all same-prefix, of course.

Right, these are all literals that got de-duped by the compiler. That's not something I'd normally rely on, but in this case, missing a few duplicates wouldn't matter that much, so it doesn't seem worth doing string compares rather than pointer compares.

Having a single prefix for all CSS probably does make sense. If nothing else, it would simplify things here, and be many fewer strings for the pref callback matchers to compare. I'll file a follow-up.
The compiler may dedup string literals in one compilation unit, but there's no guarantee that the linker will dedup them across compilation units. As a matter of fact, this is guaranteed to *not* happen in local builds.
(In reply to Mike Hommey [:glandium] from comment #6)
> The compiler may dedup string literals in one compilation unit, but there's
> no guarantee that the linker will dedup them across compilation units. As a
> matter of fact, this is guaranteed to *not* happen in local builds.

Hence "That's not something I'd normally rely on"
Comment on attachment 8992122 [details]
Bug 1473634: Part 1 - Add preference callback variant which matches multiple prefs.

https://reviewboard.mozilla.org/r/257000/#review263936
Attachment #8992122 - Flags: review?(n.nethercote) → review+
Comment on attachment 8992123 [details]
Bug 1473634: Part 2 - Update some callers to use RegisterCallbacks where appropriate.

https://reviewboard.mozilla.org/r/257002/#review263938

+1 for reduction in code size and duplication.

::: dom/media/CubebUtils.cpp:540
(Diff revision 1)
> +};
> +static const char* gCallbackPrefs[] = {
> +  // We don't want to call the callback on startup, because the pref is the
> +  // empty string by default ("", which means "logging disabled"). Because the
> +  // logging can be enabled via environment variables (MOZ_LOG="module:5"),
> +  // calling this callback on init would immediately re-disable the logging.

This comment applies to `PREF_CUBEB_LOGGING_LEVEL`; please move it so it's immeidately above that list element.
Attachment #8992123 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee9a6fb27eb19852c21ecfa035a2340894b2659
Bug 1473634: Part 1 - Add preference callback variant which matches multiple prefs. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/81df12079942560a70b45c2fb4d38a893a49e7fd
Bug 1473634: Part 2 - Update some callers to use RegisterCallbacks where appropriate. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/2793704b84bb08f95a8fc4e41becede57455e012
Bug 1473634: Part 3 - Update nsDOMComputedStyle to use RegisterCallbacks. r=bz
You need to log in before you can comment on or make changes to this bug.