1449395 - Remove nsStaticAtomSetup

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

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 :(

Comment 2

[Nicholas Nethercote [inactive]]

> 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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1449395 - Remove unnecessary nsStaticAtom.h includes.

For nsCSSAnonBoxes.cpp, nsCSSPseudoElements.cpp, nsDirectoryService.cpp, the
corresponding .h file includes nsStaticAtom.h. For the other files in this
patch, nsStaticAtom.h is not needed at all.

Review commit: https://reviewboard.mozilla.org/r/232398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232398/

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1449395 - Remove nsStaticAtomSetup.

Each nsStaticAtomSetup contains a pointer to a static atom, and also a pointer
to the canonical pointer to that static atom. Which is pretty weird! The
notable thing thing about it is that these structs are in an array, and that
gives us the only way to iterate over all static atoms in a single class, for
registration and lookups.

But thanks to various other recent changes to the implementation of static
atoms, we can now put the static atoms themselves into an array, which can be
iterated over. So this patch does that. With that done, nsStaticAtomSetup is no
longer necessary.

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

- The bss reduction is one word per atom, because the canonical static atom
  pointers (e.g. nsGkAtoms::foo) have moved from .bss to .data, because they're
  now initialized at compile time instead of runtime.

- The data reduction is one word per atom, because we remove two words per atom
  for the nsStaticAtomSetup removal, but gain one word per atom from the
  previous bullet point.

- I'm not sure about the text reduction. It's three words per atom. Maybe
  because there is one less relocation per atom?

Other notable things in the patch:

- nsICSSAnonBoxPseudo and nsICSSPseudoElement now inherit from nsStaticAtom,
  not nsAtom, because that's more precise.

- Each static atoms array now has an enum associated with it, which is used in
  various ways.

- In the big comment about the macros at the top of nsStaticAtom.h, the pre-
  and post-expansion forms are now shown interleaved. The interleaving reduces
  duplication and makes the comment much easier to read and maintain. The
  comment also has an introduction that explains the constraints and goals of
  the implementation.

- The SUBCLASS macro variations are gone. There are few enough users of these
  macros now that always passing the atom type has become simpler.

Review commit: https://reviewboard.mozilla.org/r/232400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232400/

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8963465 [details]
Bug 1449395 - Remove unnecessary nsStaticAtom.h includes.

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

r=me assuming this compiles.

Comment 6

[Nathan Froyd [:froydnj]]

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. :(

Comment 7

[Nicholas Nethercote [inactive]]

> 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().

Comment 8

[Nathan Froyd [:froydnj]]

(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`?

Comment 9

[Nicholas Nethercote [inactive]]

I did some local opt builds. On Mac, `XUL` shrunk by 58448 bytes. On Win64, `xul.dll` shrunk by 50176 bytes.

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/554acd814741bf777b6312b027fb9108c97d0c1b
Bug 1449395 - Remove unnecessary nsStaticAtom.h includes. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/9111840008a1707bfabc031a66612436d71135b0
Bug 1449395 - Remove nsStaticAtomSetup. r=froydnj

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/554acd814741
https://hg.mozilla.org/mozilla-central/rev/9111840008a1