Closed Bug 1449395 Opened 2 years ago Closed 2 years ago

Remove nsStaticAtomSetup

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Static atoms currently look like this:

> const char16_t[7] (.rodata, 2(N+1) bytes)
> (this is detail::gFooAtoms.foobar_string)
> /-----------------------------------------\ <-------+
> | u"foobar"           // the actual chars |         |
> \-----------------------------------------/         |
>                                                     |
> const nsStaticAtom (.rodata, 12 bytes)              |
> (this is detail::gFooAtoms.foobar_atom)             |
> /-------------------------------------\ <--------+  |
> | uint32_t mLength:30 = 6             |          |  |
> | uint32_t mKind:2 = AtomKind::Static |          |  |
> | uint32_t mHash = ...                |          |  |
> | uint32_t mStringOffset = @----------|----------|--+
> \-------------------------------------/          |
>                                                  |
> static nsAtom* (.bss, 8 bytes)                   |
> (this is MyAtoms::foobar)                        |
> /---\ <---------------------------------------+  |
> | @-|-----------------------------------------|--+
> \---/                                         |  |
>                                               |  |
> static nsStaticAtomSetup (.d.r.ro, 16 bytes)  |  |
> (this is an element in sMyAtomsSetup[])       |  |
> /-----------------------------------\         |  |
> | nsStaticAtom* const mAtom = @-----|---------|--+
> | nsStaticAtom** const mAtomp = @---|---------+  |
> \-----------------------------------/            |
>                                                  |
> AtomTableEntry (heap, ~2 x 16 bytes[*])          |
> (this entry is part of gAtomTable)               |
> /-------------------------\                      |
> | uint32_t mKeyHash = ... |                      |
> | nsAtom* mAtom = @-------|----------------------+
> \-------------------------/
> 
> [*] Each hash table is half full on average, so each entry takes up
> approximately twice its actual size.
> 
> static shared bytes: 12 + 2(N+1)
> static unshared bytes: 8 + 16 = 24
> heap bytes: ~32
> total bytes: 12 + 2(N+1) + ~56 * num_processes

This bug is about getting them to this:

> const char16_t[7] (.rodata, 2(N+1) bytes)
> (this is detail::gFooAtoms.foobar_string)
> /-----------------------------------------\ <-------+
> | u"foobar"           // the actual chars |         |
> \-----------------------------------------/         |
>                                                     |
> const nsStaticAtom (.rodata, 12 bytes)              |
> (this is within detail::gFooAtoms.mAtoms[])         |
> /-------------------------------------\ <---+       |
> | uint32_t mLength:30 = 6             |     |       |
> | uint32_t mKind:2 = AtomKind::Static |     |       |
> | uint32_t mHash = ...                |     |       |
> | uint32_t mStringOffset = @----------|-----|-------+
> \-------------------------------------/     |
>                                             |
> constexpr nsStaticAtom* (0 bytes) @---------+
> (this is MyAtoms::foobar)                   |
>                                             |
> AtomTableEntry (heap, ~2 x 16 bytes[*])     |
> (this entry is part of gAtomTable)          |
> /-------------------------\                 |
> | uint32_t mKeyHash = ... |                 |
> | nsAtom* mAtom = @-------|-----------------+
> \-------------------------/
> 
> [*] Each hash table is half full on average, so each entry takes up
> approximately twice its actual size.
> 
> static shared bytes: 12 + 2(N+1)
> static unshared bytes: 0
> dynamic bytes: ~32
> total bytes: 12 + 2(N+1) + ~32 * num_processes

I.e. the nsSaticAtomSetup is gone, and the per-atom pointers are constexpr so they don't take up any space.
I have a draft patch for this that is working on Linux and Mac to the point that the browser starts up ok. (I haven't tested more than that.)

But MSVC has an inability to correctly compute `&foo[N]` in a constexpr context:

> https://developercommunity.visualstudio.com/content/problem/223146/constexpr-code-gneration-bug.html

and I'm having trouble coming up with a workaround :(
Depends on: 1449414
Depends on: 1449787
> But MSVC has an inability to correctly compute `&foo[N]` in a constexpr context

I'm going to deal with this, for now, by removing nsStaticAtomSetup but not making the individual atom pointers `constexpr`. I.e. instead of this from comment 0:

> constexpr nsStaticAtom* (0 bytes)
> (this is MyAtoms::foobar)

We will have this:

> nsStaticAtom* (.data 8 bytes)
> (this is MyAtoms::foobar)

This will still be a net win. Also, I'll be able to break up what would have been a single mega-patch into more manageable chunks, which is nice.

I have filed bug 1449787 for making the individual atom pointers `constexpr`, once Microsoft fixes the MSVC bug.
Blocks: 1449787
No longer depends on: 1449787
Blocks: 1449827
Blocks: 1449883
Comment on attachment 8963465 [details]
Bug 1449395 - Remove unnecessary nsStaticAtom.h includes.

https://reviewboard.mozilla.org/r/232398/#review238004

r=me assuming this compiles.
Attachment #8963465 - Flags: review?(nfroyd) → review+
Comment on attachment 8963466 [details]
Bug 1449395 - Remove nsStaticAtomSetup.

https://reviewboard.mozilla.org/r/232400/#review238008

::: commit-message-7478c:14
(Diff revision 1)
> +According to the `size` utility, on Linux64 this reduces the size of libxul.so
> +by the following amounts:
> +
> +> text:  62008 bytes
> +> data:  20992 bytes
> +> bss:   21040 bytes
> +> total: 104040 bytes

Do these size improvements hold for xul on Mac and Windows as well?

::: commit-message-7478c:30
(Diff revision 1)
> +- I'm not sure about the text reduction. It's three words per atom. Maybe
> +  because there is one less relocation per atom?

x86-64 Linux relocations are stupidly large, sadly, so this is probably correct.

::: layout/style/nsCSSAnonBoxes.h:15
(Diff revision 1)
> -// Empty class derived from nsAtom so that function signatures can
> -// require an atom from this atom list.
> -class nsICSSAnonBoxPseudo : public nsAtom {};
> +// Trivial subclass of nsStaticAtom so that function signatures can require an
> +// atom from this atom list.
> +class nsICSSAnonBoxPseudo : public nsStaticAtom

We should have a `static_assert` somewhere that asserts the size of these two classes are the same, so we can be assured that casting `&mAtoms[0]` to a `nsStaticAtom*` will enable us to correctly access members of `mAtoms`.  Preferably with an explanatory comment.

::: layout/style/nsCSSPseudoElements.h:59
(Diff revision 1)
> +// Trivial subclass of nsStaticAtom so that function signatures can require an
> +// atom from this atom list.
> +class nsICSSPseudoElement : public nsStaticAtom

Same `static_assert` comment applies to this subclass as well.

::: xpcom/ds/nsStaticAtom.h
(Diff revision 1)
> -template<uint32_t N>
>  void
> -NS_RegisterStaticAtoms(const nsStaticAtomSetup (&aSetup)[N])
> +NS_RegisterStaticAtoms(const nsStaticAtom* aAtoms, size_t aAtomsLen);

I am not super happy about losing this, but I'm not sure that we can continue with the template magic with the current setup. :(
Attachment #8963466 - Flags: review?(nfroyd) → review+
> Do these size improvements hold for xul on Mac and Windows as well?

I haven't checked. Do you know how to make similar measurements on Mac and Windows?


> > -NS_RegisterStaticAtoms(const nsStaticAtomSetup (&aSetup)[N])
> > +NS_RegisterStaticAtoms(const nsStaticAtom* aAtoms, size_t aAtomsLen);
> 
> I am not super happy about losing this, but I'm not sure that we can
> continue with the template magic with the current setup. :(

I think we could, but it would require passing mozilla::detail::gGkAtoms.mAtoms[] instead of sAtoms, and elsewhere we'd have to use mozilla::ArrayLength(mozilla::detail::gCSSPseudoElementAtoms.mAtoms) instead of sAtomsLen, and I figured the shorter names were nicer. Plus they let me remove the `extern` function declaration within NS_RegisterStaticAtoms().
Flags: needinfo?(nfroyd)
(In reply to Nicholas Nethercote [:njn] from comment #7)
> > Do these size improvements hold for xul on Mac and Windows as well?
> 
> I haven't checked. Do you know how to make similar measurements on Mac and
> Windows?

You can look at the build logs (for Windows, at least) and search for "Size of xul.dll".  Unfortunately the build process doesn't produce the size numbers for Mac as well; I guess you could install the Mac binaries from different builds into temporary directories and `find $DIR -name XUL | xargs ls -l`?
Flags: needinfo?(nfroyd)
I did some local opt builds. On Mac, `XUL` shrunk by 58448 bytes. On Win64, `xul.dll` shrunk by 50176 bytes.
https://hg.mozilla.org/mozilla-central/rev/554acd814741
https://hg.mozilla.org/mozilla-central/rev/9111840008a1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → n.nethercote
Blocks: 1451169
You need to log in before you can comment on or make changes to this bug.