Closed Bug 1455178 Opened 6 years ago Closed 6 years ago

gGkAtoms generates a static initializer with MSVC, and is writable

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Examining libxul symbols on Windows today for a different bug, I noticed:

101039 code mozilla::detail::`dynamic initializer for 'gGkAtoms''

and

87544 data mozilla::detail::gGkAtoms

Which kinda negates the whole point of having static static atoms.  (These are both top 20 symbols in libxul; I guess having gGkAtoms there is unavoidable, but having a 100k static initializer seems like a failure...)

I assume this is related to having several hundreds of atoms in GkAtoms, because I don't seen static initializers for other atoms.  Can we attempt to come up with an alternative for initializing GkAtoms that generates less code?
Flags: needinfo?(n.nethercote)
I tried poking at this today, first by attempting to move the hashing bits out into separate variables.  The working theory here was that by making the hash function evaluations their own individual variables, they would be more likely to constant-fold into something.  And each separate variable would receive its own constexpr evaluation budget (assuming that the budget is actually per-expression, rather than something weird like per-file), which means that there would (theoretically) be more budget for initializing gGkAtoms.

That did not work; the static initializer is still there.  (I think it's actually *bigger* than it was before, even. =/)

The second idea starts by noticing that we told MSVC to use ~300K evaluation steps for compiling nsGkAtoms.cpp.  But we have ~3000 atoms in nsGkAtoms.cpp, and ~6000 members in `struct GkAtoms`.  So that's ~50 evaluation steps for each member of `struct GkAtoms`, which is potentially not that much.  Maybe if we cranked the limit up a bit?

Going to 1M steps didn't help.  I just pushed 3M steps, but I'm not super optimistic that will have any effect.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcff1d580267b428d83d88ce87779e975a8b9203

For comparison purposes, the try pushes have been based off of:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=0e45c13b34e815cb42a9f08bb44142d1a81e186e
I made the static constructor smaller by changing GkAtoms to have individual nsStaticAtom members, rather than an array of such members.  But it didn't remove the constructor completely, and the constructor is still 80k in any event.

Next guess is that offsetof calls in the code are hindering things somehow?  I need to write a slimmed-down testcase that demonstrates the effect and send it to Microsoft.
(In reply to Nathan Froyd [:froydnj] from comment #2)
> I made the static constructor smaller by changing GkAtoms to have individual
> nsStaticAtom members, rather than an array of such members.

How does that work? One of the fundamental constraints, listed in the comment at the top of nsStaticAtom.h, is this:

// - We need them to be in an array, so we can iterate over them (for
//   registration and lookups).
Flags: needinfo?(n.nethercote) → needinfo?(nfroyd)
(In reply to Nicholas Nethercote [:njn] from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > I made the static constructor smaller by changing GkAtoms to have individual
> > nsStaticAtom members, rather than an array of such members.
> 
> How does that work? One of the fundamental constraints, listed in the
> comment at the top of nsStaticAtom.h, is this:
> 
> // - We need them to be in an array, so we can iterate over them (for
> //   registration and lookups).

You can do this:

struct Foo
{
  const nsStaticAtom dummy;

  ...list out const nsStatic members...
};

const nsStaticAtom* const sAtoms = &gFooAtoms.dummy + 1;

I didn't know a good way to get at the first atom (something the array solution does nicely handle), so the dummy member is a bit of a hack.

A little thought says that laying members out this way must be identical to having them in an array.  So the requirement is better stated as contiguous memory, rather than an array.
Flags: needinfo?(nfroyd)
For reasons unknown, if you give MSVC:

// Foo.h
struct Foo
{
  ...
};

extern const Foo gFoo;

// Foo.cpp, which necessarily includes Foo.h.
extern constexpr Foo gFoo = {
};

MSVC will create a static initializer for gFoo and place it in the
read/write data section, rather than the read-only data section.
Removing the `extern const` declaration seems to be enough to make this
problem go away.  We need to adjust the declaration of other variables
to compensate for the non-visibility of gFoo in the header file.
Attachment #8969926 - Flags: review?(n.nethercote)
Comment on attachment 8969926 [details] [diff] [review]
avoid static constructors for atom initialization

Review of attachment 8969926 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for digging into this. I don't think I would have worked out this fix.

I checked and confirmed that gGkAtoms et al. are still in .rodata on Linux.

I like that the new code is shorter and simpler than the old code :)  I also wonder if this change would help fix some of the problems blocking bug 1449787.
Attachment #8969926 - Flags: review?(n.nethercote) → review+
BTW, how did you do the investigation described in comment 0?
Flags: needinfo?(nfroyd)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e61303bec3eb
avoid static constructors for atom initialization; r=njn
https://hg.mozilla.org/mozilla-central/rev/e61303bec3eb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Nicholas Nethercote [:njn] from comment #7)
> BTW, how did you do the investigation described in comment 0?

I ran SymbolSort:

https://github.com/adrianstone55/SymbolSort

with a fix for busted Rust debug info that I'm trying to decide whether I should submit a pull request for.  It shows you all the symbols, nicely demangled, with an indication of what section they're in...and I think some other useful information besides.
Assignee: nobody → nfroyd
Flags: needinfo?(nfroyd)
You need to log in before you can comment on or make changes to this bug.