Closed Bug 1449787 Opened 2 years ago Closed Last year

Make static atom pointers `constexpr`

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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.
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.
No longer blocks: 1449395
Depends on: 1449395
Depends on: 1449827
Depends on: 1449883
Blocks: 1257126
Depends on: 1451169
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.
It's possible that we'll switch from MSVC to clang before the bug is fixed.
Depends on: 1490654
At least the problem linked in comment #0 did not happen on MSVC 15.8.4. Can you restart the work?
Flags: needinfo?(n.nethercote)
I'll take a look sometime soon. Thanks for the info.
Flags: needinfo?(n.nethercote)
The patch also changes sSystemMetrics to not refcount its elements, because
static atoms don't need refcounting.

MozReview-Commit-ID: 7uf7YjyNaZp
Attachment #9013221 - Flags: review?(emilio)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Specifically, give all the string templates the ''' form, and give them all
`_TEMPLATE` suffixes. This requires slightly changing the newline handling.
Attachment #9013222 - Flags: review?(emilio)
emilio, please review the regen_atoms.py changes and the *.rs changes.

froydnj, please review everything else.
Attachment #9013223 - Flags: review?(nfroyd)
Attachment #9013223 - Flags: review?(emilio)
In case it helps, here's the new generated atom_macro.rs file.
Attachment #9013224 - Attachment mime type: text/rust → text/plain
Attachment #9013221 - Flags: review?(emilio) → review+
Attachment #9013222 - Flags: review?(emilio) → review+
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
Attachment #9013223 - Flags: review?(emilio) → review+
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?
Attachment #9013223 - Flags: review?(nfroyd)
> 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.
> > +    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.
>> +// 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.
Flags: needinfo?(nfroyd)
froydnj, here is a rolled-up patch for this bug and the two blocking bugs, in case you want to play with this yourself.
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>
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.)
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?)
Flags: needinfo?(nfroyd)
Thank you for the suggestions. I've applied them, it looks much better.
Attachment #9014224 - Flags: review?(nfroyd)
Attachment #9013223 - Attachment is obsolete: true
Attachment #9014224 - Flags: review?(nfroyd) → review+
Pushed by nnethercote@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.