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

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla27
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

6 years ago
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])
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.
Assignee

Comment 2

6 years ago
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.
Assignee

Comment 3

6 years ago
Posted patch fix v1Splinter Review
Attachment #806955 - Flags: review?(matspal)
(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 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
Attachment #806955 - Flags: review?(matspal) → review+
Assignee

Comment 6

6 years ago
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). :)
Summary: For static variables in nsLayoutUtils.cpp that only get used in one function, move them to that function's scope → For null-initialized static variables in nsLayoutUtils.cpp that only get used in one function, move them to that function's scope
Assignee

Comment 7

6 years ago
Tree's closed; using checkin-needed.
Keywords: checkin-needed
Assignee

Comment 8

6 years ago
Tree reopened! Landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8367f4c4fb71
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8367f4c4fb71
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.