918109 - For null-initialized static variables in nsLayoutUtils.cpp that only get used in one function, move them to that function's scope

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

While reviewing attachment 802685 [details] [diff] [review] (which adds a function much like FlexboxEnabledPrefChangeCallback), I was inspired to move FlexboxEnabledPrefChangeCallback's static variables into its function scope.

Patch coming up to do that, and do the same for a few other static variables. (and also adds an assertion analogous to the assertion I requested on attachment 802685 [details] [diff] [review])

Comment 1

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

function-scope static variables are more expensive than global-scope ones (at least if they require any non-null initialization), since they're initialized the first time the function is called, which requires an extra hidden (compiler-generated) boolean to guard.

Comment 2

[Daniel Holbert [:dholbert]]

Ah, good to know - thanks!

Nearly all of the variables I was going to move use null initialization (or equivalent, e.g. "= false", which could be removed if it actually imposes a cost).

I'll hold off on moving sScrollIdCounter, though, since that one's initialized to 2.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 4

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #1)
> function-scope static variables are more expensive than global-scope ones

That's a good point, but these pref callbacks are only called once on startup, and when the pref
changes, so I don't think we need to worry about performance in this case.

If we put the sScrollIdCounter definition inside the "if (!FindIDFor(aContent, &scrollId))"
we will only do the guard check once per content object the method is called with, which I think
is negligible.
http://mxr.mozilla.org/mozilla-central/search?string=FindOrCreateIDFor

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 806955 [details] [diff] [review]
fix v1

If you agree with my last comment, then please move the sScrollIdCounter
inside the 'if'.

r=mats

Comment 6

[Daniel Holbert [:dholbert]]

I'll leave sScrollIdCounter alone for now, since it's doing something different and it's not as obviously-worth-fixing as the other variables.

So, I'll just land the patch as-is. If anyone cares about sScrollIdCounter being function-scoped, we can make that change in a separate bug (or separate patch on this bug). :)

Comment 7

[Daniel Holbert [:dholbert]]

Tree's closed; using checkin-needed.

Comment 8

[Daniel Holbert [:dholbert]]

Tree reopened! Landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8367f4c4fb71

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8367f4c4fb71