1449787 - Make static atom pointers `constexpr`

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

Once bug 1449395 is done, we'll be very close to the minimal static atom setup. The only thing left will be that the individual static atom pointers (nsGkAtoms::foo, etc.) still take up space. It's possible to make them `constexpr` and thus take up no space, except that MSVC has a bug that prevents this from happening:

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

Once that MSVC bug is fixed, this bug can be implemented.

Comment 1

[Nicholas Nethercote [inactive]]

Note: we have a ton of static data structures that have references to static atoms like this:

> &nsGkAtoms::foo

Why is the '&' there? Because static atoms used to not be statically allocated, and so couldn't be pointed to directly by other static data structures. Instead the static data structures would point to a pointer to the atom, which is static. (This means all uses required an additional dereference.)

But bug 1411469 made atoms statically allocated, and bug 1445113 will disallow duplicate static atoms, whereupon it will be be both possible and valid for static data structures to refer directly to the static atoms. Indeed, in order to complete *this* bug, all these pointers-to-pointers must be changed to direct pointers, because all the nsGkAtoms::foo pointers will become constexpr, which means they are rvalues rather than lvalues, and so doing &nsGkAtoms::foo will be impossible.

So this bug will have a bunch of blockers doing those nsStaticAtom** to nsStaticAtom* conversions.

Comment 2

[Nicholas Nethercote [inactive]]

Here's a frustrating thing: bug 1449883 and bug 1451169 are precursors to this bug. They cause the number of static constructors to go up, by about 17 or so. (Bug 1451169's patches were backed out for this reason.) This bug will then reverse the static constructor increase.

Therefore, I can't land this bug's precursors gradually. They'll have to land all at once, when this bug also lands. And that's blocked by the MSVC bug described in comment 0. Sigh.

Comment 3

[Nicholas Nethercote [inactive]]

It's possible that we'll switch from MSVC to clang before the bug is fixed.

Comment 4

[Masatoshi Kimura [:emk]]

At least the problem linked in comment #0 did not happen on MSVC 15.8.4. Can you restart the work?

Comment 5

[Nicholas Nethercote [inactive]]

I'll take a look sometime soon. Thanks for the info.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Reduce indirection for static pointers in nsMediaFeature

The patch also changes sSystemMetrics to not refcount its elements, because
static atoms don't need refcounting.

MozReview-Commit-ID: 7uf7YjyNaZp

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Tweak regen_atoms.py

Specifically, give all the string templates the ''' form, and give them all
`_TEMPLATE` suffixes. This requires slightly changing the newline handling.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Make static atom pointers `constexpr`

emilio, please review the regen_atoms.py changes and the *.rs changes.

froydnj, please review everything else.

Comment 9

[Nicholas Nethercote [inactive]]

Try looks good, even the tier 2 MSVC jobs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62eca49f721c3c509d5f19464838e0febd603380

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Generated atom_macro.rs

In case it helps, here's the new generated atom_macro.rs file.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9013223 [details] [diff] [review]
Make static atom pointers `constexpr`

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

rust bits and regen_atoms.py look fine to me, thanks!

::: servo/components/style/gecko/regen_atoms.py
@@ +157,4 @@
>  '''
>  
> +CONST_TEMPLATE = '''
> +pub const k_{name}: isize = {index};

Can we put the #[allow()] here instead?

@@ +183,3 @@
>          f.write(UNSAFE_STATIC)
>  
> +        gnu_name='_ZN9nsGkAtoms6sAtomsE'

I think we should try to use the bindgen-generated version of this, which would allow the manual symbol stuff go away. But I guess this is fine as well, for now.

::: xpcom/ds/nsGkAtoms.h
@@ +171,5 @@
>    #include "nsGkAtomList.h"
>    #undef GK_ATOM
>  };
>  
> +// XXX: GCC somehow miscompiles expressions such as `cond ? nsGkAtoms::a :

ugh

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 9013223 [details] [diff] [review]
Make static atom pointers `constexpr`

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

I would really like to understand all the casting nonsense better before giving this r+.  Otherwise this looks fine.  Did you confirm that sizes etc. go down as expected?

::: layout/style/nsCSSAnonBoxes.h
@@ -95,5 @@
>  #endif
>  
>    // Alias nsCSSAnonBoxes::foo() to alias nsGkAtoms::AnonBox_foo.
> -  // XXX Once nsGkAtoms::AnonBox_foo become constexpr variables, these can too.
> -  // See bug 1449787.

Please file a followup for this.

::: layout/style/nsCSSPseudoElements.h
@@ -104,5 @@
>  #endif
>  
>    // Alias nsCSSPseudoElements::foo() to alias nsGkAtoms::foo.
> -  // XXX Once nsGkAtoms::foo become constexpr variables, these can too.
> -  // See bug 1449787.

Likewise for this, or roll them into the same bug.

::: layout/style/nsMediaFeatures.cpp
@@ +316,5 @@
>    int32_t metricResult =
>      LookAndFeel::GetInt(LookAndFeel::eIntID_ScrollArrowStyle);
>    if (metricResult & LookAndFeel::eScrollArrow_StartBackward) {
> +    sSystemMetrics->AppendElement(
> +      (nsStaticAtom*)nsGkAtoms::_moz_scrollbar_start_backward);

Hm, you explained the reason for this, but can you provide some of the errors that you get?  Is this cast getting rid of constness or something like that?

::: xpcom/ds/nsGkAtoms.h
@@ +171,5 @@
>    #include "nsGkAtomList.h"
>    #undef GK_ATOM
>  };
>  
> +// XXX: GCC somehow miscompiles expressions such as `cond ? nsGkAtoms::a :

This is pretty gross.  What does the generated assembly look like here for the code without the casts?

Comment 13

[Nicholas Nethercote [inactive]]

> Did you confirm that sizes etc. go down as expected?

Yes. It reduces binary size on Linux64 by 51200 bytes, and the nsGkAtoms::foo entries are gone from the .data section.


> >    // Alias nsCSSAnonBoxes::foo() to alias nsGkAtoms::AnonBox_foo.
> > -  // XXX Once nsGkAtoms::AnonBox_foo become constexpr variables, these can too.
> > -  // See bug 1449787.
> 
> Please file a followup for this.

This comment was inaccurate to start with -- the function already is `constexpr`, even though the comment suggests it is not. However, I had to make the function non-`constexpr`, because you can't downcast from `nsStaticAtom*` to `nsCSSAnonBoxes*` in a `constexpr` expression. It's not a big deal that this function isn't constexpr, though. 

All this means there is no need for a follow-up.

Comment 14

[Nicholas Nethercote [inactive]]

> > +    sSystemMetrics->AppendElement(
> > +      (nsStaticAtom*)nsGkAtoms::_moz_scrollbar_start_backward);
> 
> Hm, you explained the reason for this, but can you provide some of the
> errors that you get?  Is this cast getting rid of constness or something
> like that?

In a debug build with clang, I get this link error:

> /usr/bin/ld.lld: error: undefined symbol: nsGkAtoms::_moz_scrollbar_start_backward
> >>> referenced by Unified_cpp_layout_style3.cpp
> >>>               ../../layout/style/Unified_cpp_layout_style3.o:(nsMediaFeatures::InitSystemMetrics())

An opt build with clang works fine.

In a debug or opt build with GCC, I get this link error:

> /usr/bin/x86_64-linux-gnu-ld.gold: error: /home/njn/moz/mi2/gd64/toolkit/library/../../layout/style/Unified_cpp_layout_style3.o: requires dynamic R_X86_64_PC32 reloc against '_ZN9nsGkAtoms29_moz_scrollbar_start_backwardE' which may overflow at runtime; recompile with -fPIC
> /usr/bin/x86_64-linux-gnu-ld.gold: error: read-only segment has dynamic relocations
> /home/njn/moz/mi2/gd64/dist/include/nsTArray.h:542: error: undefined reference to 'nsGkAtoms::_moz_scrollbar_start_backward'

AFAICT, the use of any kind of cast, to `nsStaticAtom*` or `const nsStaticAtom*` or `nsStaticAtom* const`, be it a C-style cast or a `static_cast` or a `const_cast`, is enough to fix the problem. Introducing any kind of local variable also fixes the problem. So `(nsStaticAtom*)` is the most concise way to work around it.

Comment 15

[Nicholas Nethercote [inactive]]

>> +// XXX: GCC somehow miscompiles expressions such as `cond ? nsGkAtoms::a :
> 
> This is pretty gross.  What does the generated assembly look like here for the code without the casts?

Here's sample code for the version that compiles ok:

> nsAtom*
> nsSVGBoolean::GetBaseValueAtom() const      
> { 
>   return MOZ_CHOOSE_STATIC_ATOM(mBaseVal, nsGkAtoms::_true, nsGkAtoms::_false);
> }          
> 
> 0000000000002c42 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv>:
>     2c42:       55                      push   %rbp
>     2c43:       48 89 e5                mov    %rsp,%rbp
>     2c46:       80 7f 01 00             cmpb   $0x0,0x1(%rdi)
>     2c4a:       48 8d 05 00 00 00 00    lea    0x0(%rip),%rax        # 2c51 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0xf>
>     2c51:       48 8d 90 e4 dc ff ff    lea    -0x231c(%rax),%rdx
>     2c58:       48 0f 44 c2             cmove  %rdx,%rax
>     2c5c:       5d                      pop    %rbp
>     2c5d:       c3                      retq

Here's sample code for the version with the link error:

> nsAtom*
> nsSVGBoolean::GetBaseValueAtom() const      
> { 
>   return mBaseVal ? nsGkAtoms::_true : nsGkAtoms::_false;
> }          
> 
> 0000000000002c42 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv>:
>     2c42:       55                      push   %rbp
>     2c43:       48 89 e5                mov    %rsp,%rbp
>     2c46:       48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 2c4d <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0xb>
>     2c4d:       80 7f 01 00             cmpb   $0x0,0x1(%rdi)
>     2c51:       75 02                   jne    2c55 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0x13>
>     2c53:       5d                      pop    %rbp
>     2c54:       c3                      retq
>     2c55:       48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 2c5c <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0x1a>
>     2c5c:       eb f5                   jmp    2c53 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0x11>

I don't entirely understand either of them, but the duplication of `mov 0x0(%rip),%rax` on two paths in the latter is really weird.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Rolled up changes

froydnj, here is a rolled-up patch for this bug and the two blocking bugs, in case you want to play with this yourself.

Comment 17

[Nicholas Nethercote [inactive]]

With relocations...

The MOZ_CHOOSE_STATIC_ATOM version:

> 0000000000002c42 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv>:
>    2c42:      55                      push   %rbp
>    2c43:      48 89 e5                mov    %rsp,%rbp
>    2c46:      80 7f 01 00             cmpb   $0x0,0x1(%rdi)
>    2c4a:      48 8d 05 00 00 00 00    lea    0x0(%rip),%rax        # 2c51 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0xf>
>                       2c4d: R_X86_64_PC32     _ZN7mozilla6detail8gGkAtomsE+0x103b4
>    2c51:      48 8d 90 e4 dc ff ff    lea    -0x231c(%rax),%rdx
>    2c58:      48 0f 44 c2             cmove  %rdx,%rax
>    2c5c:      5d                      pop    %rbp
>    2c5d:      c3                      retq   

Note that the `_false` atom does appear within `gGkAtoms` 0x231c bytes before the `_true` atom, so that's where that number comes from.

The ?: version:

> 0000000000002c42 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv>:
>    2c42:      55                      push   %rbp
>    2c43:      48 89 e5                mov    %rsp,%rbp
>    2c46:      48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 2c4d <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0xb>
>                       2c49: R_X86_64_PC32     _ZN9nsGkAtoms6_falseE-0x4
>    2c4d:      80 7f 01 00             cmpb   $0x0,0x1(%rdi)
>    2c51:      75 02                   jne    2c55 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0x13>
>    2c53:      5d                      pop    %rbp 
>    2c54:      c3                      retq   
>    2c55:      48 8b 05 00 00 00 00    mov    0x0(%rip),%rax        # 2c5c <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0x1a>
>                       2c58: R_X86_64_PC32     _ZN9nsGkAtoms5_trueE-0x4
>    2c5c:      eb f5                   jmp    2c53 <_ZNK12nsSVGBoolean16GetBaseValueAtomEv+0x11>

Comment 18

[Nicholas Nethercote [inactive]]

So, with `?:` GCC generates references to `nsGkAtoms::_true` and `nsGkatoms::_false`, but they are `constexpr` and so can't be referred to like that. With `MOZ_CHOOSE_STATIC_ATOM` GCC instead indexes into `gGkAtoms`, which is reasonable (and what clang does in both opt and debug builds). The problem occurred everywhere that static atoms were referenced within the `?:` operator, so my guess is it's a GCC bug. (And note that the problem occurs with GCC in both debug and opt builds.)

Comment 19

[Nathan Froyd [:froydnj]]

It is a GCC bug, see:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87494

A better workaround which eliminates the need for MOZ_CHOOSE_STATIC_ATOM:

https://hg.mozilla.org/try/rev/8ab5ce955453949c882e00fa02942de8524674a5

That patch makes accesses to atoms slightly less efficient, but I think I will take that tradeoff for not cluttering up the source code.  Make that change, with comments about NS_EXTERNAL_VIS being a workaround for the aforementioned GCC bug, revert all the MOZ_CHOOSE_STATIC_ATOM changes, and we can land this?

(I did try removing the casts for the AppendElement cases, but that didn't work, so...maybe that's something else?)

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: Make static atom pointers `constexpr`

Thank you for the suggestions. I've applied them, it looks much better.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77803e121c8f
Reduce indirection for static pointers in nsMediaFeature. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/66b689d8aa4d
Tweak regen_atoms.py. r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/41216e689dda
Make static atom pointers `constexpr`. r=froydnj,emilio

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/77803e121c8f
https://hg.mozilla.org/mozilla-central/rev/66b689d8aa4d
https://hg.mozilla.org/mozilla-central/rev/41216e689dda