1451169 - Improve static atom handling in various places

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

Now that bug 1449395 has landed, we can convert most of the remaining &nsGkAtoms::foo occurrences in nsTreeSanitizer to nsGkAtoms::foo, as described in bug 1449787 comment 1.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Change nsRoleMapEntry::roleAtom from `nsStaticAtom**` to `nsStaticAtom* const`.

And likewise for AttrCharacteristics::attributeName.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in nsAccessibilityService.h.

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

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Use `nsStaticAtom* const` instead of `nsStaticAtom**` in DocAccessible.cpp.

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

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in Element.h.

And then fix up everything else that needs to change as well.

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

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Reduce indirection for static atom pointers in nsCSSFrameConstructor.h.

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

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Reduce indirection for FrameConstructionDataByTag::mTag.

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

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Reduce indirection for PseudoParentData::mPseudoType.

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

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Bug 1451169 - Reduce indirection for static pointers in txCoreFunctionCall.cpp.

The patch also adds more `const` to txCoreFunctionDescriptor.

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

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8964757 [details]
Bug 1451169 - Change nsRoleMapEntry::roleAtom from `nsStaticAtom**` to `nsStaticAtom* const`.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233480/diff/1-2/

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8964758 [details]
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in nsAccessibilityService.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233482/diff/1-2/

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8964759 [details]
Bug 1451169 - Use `nsStaticAtom* const` instead of `nsStaticAtom**` in DocAccessible.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233484/diff/1-2/

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8964760 [details]
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in Element.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233486/diff/1-2/

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8964761 [details]
Bug 1451169 - Reduce indirection for static atom pointers in nsCSSFrameConstructor.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233488/diff/1-2/

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8964762 [details]
Bug 1451169 - Reduce indirection for FrameConstructionDataByTag::mTag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233490/diff/1-2/

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 8964763 [details]
Bug 1451169 - Reduce indirection for PseudoParentData::mPseudoType.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233492/diff/1-2/

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8964764 [details]
Bug 1451169 - Reduce indirection for static pointers in txCoreFunctionCall.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233494/diff/1-2/

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8964760 [details]
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in Element.h.

https://reviewboard.mozilla.org/r/233486/#review239084

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8964761 [details]
Bug 1451169 - Reduce indirection for static atom pointers in nsCSSFrameConstructor.h.

https://reviewboard.mozilla.org/r/233488/#review239132

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8964762 [details]
Bug 1451169 - Reduce indirection for FrameConstructionDataByTag::mTag.

https://reviewboard.mozilla.org/r/233490/#review239144

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8964763 [details]
Bug 1451169 - Reduce indirection for PseudoParentData::mPseudoType.

https://reviewboard.mozilla.org/r/233492/#review239146

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8964757 [details]
Bug 1451169 - Change nsRoleMapEntry::roleAtom from `nsStaticAtom**` to `nsStaticAtom* const`.

https://reviewboard.mozilla.org/r/233480/#review239286

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8964758 [details]
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in nsAccessibilityService.h.

https://reviewboard.mozilla.org/r/233482/#review239288

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8964759 [details]
Bug 1451169 - Use `nsStaticAtom* const` instead of `nsStaticAtom**` in DocAccessible.cpp.

https://reviewboard.mozilla.org/r/233484/#review239290

Comment 24

[Eric Rahm [:erahm]]

Comment on attachment 8964764 [details]
Bug 1451169 - Reduce indirection for static pointers in txCoreFunctionCall.cpp.

https://reviewboard.mozilla.org/r/233494/#review239348

r=me assuming this compiles. Just out of curiousity, does this have any benefits outside of cleaning up code? ie, are we avoiding relocations etc?

Comment 25

[Nicholas Nethercote [inactive]]

> Just out of curiousity, does this have any
> benefits outside of cleaning up code? ie, are we avoiding relocations etc?

This blocks bug 1449787, which will save us 21 KiB of static data, and also remove about 2,600 relocations (one per static atom).

Comment 26

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/914fb7cd9fc3aa757a3145fb9e3b34d813892837
Bug 1451169 - Change nsRoleMapEntry::roleAtom from `nsStaticAtom**` to `nsStaticAtom* const`. r=davidb

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d36289e0f54548eb3305054a0d9c2e1a78a5eb0
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in nsAccessibilityService.h. r=davidb

https://hg.mozilla.org/integration/mozilla-inbound/rev/c145fbd03947d521fc621484a8f4ea65ce58c90a
Bug 1451169 - Use `nsStaticAtom* const` instead of `nsStaticAtom**` in DocAccessible.cpp. r=davidb

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb76a95897171110c6a6b3bc999ac66bc122efa5
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in Element.h. r=baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d51610ca08eeafddea2b5767a555a185a501332
Bug 1451169 - Reduce indirection for static atom pointers in nsCSSFrameConstructor.h. r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/624d824287268ce93ae31a961d7d2182a9bdf4fc
Bug 1451169 - Reduce indirection for FrameConstructionDataByTag::mTag. r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/348e825756fa69aeec071445ff4d4d664cdcbf84
Bug 1451169 - Reduce indirection for PseudoParentData::mPseudoType. r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/b92f856e15a80efb562203ca00169c06fb1f4ae4
Bug 1451169 - Reduce indirection for static pointers in txCoreFunctionCall.cpp. r=erahm

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/914fb7cd9fc3
https://hg.mozilla.org/mozilla-central/rev/6d36289e0f54
https://hg.mozilla.org/mozilla-central/rev/c145fbd03947
https://hg.mozilla.org/mozilla-central/rev/bb76a9589717
https://hg.mozilla.org/mozilla-central/rev/4d51610ca08e
https://hg.mozilla.org/mozilla-central/rev/624d82428726
https://hg.mozilla.org/mozilla-central/rev/348e825756fa
https://hg.mozilla.org/mozilla-central/rev/b92f856e15a8

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/d1db4d5030c8
Backed out 8 changesets on request from njn a=backout

Comment 29

[Andreea Pavel [:apavel]]

Backed out as requested https://bugzilla.mozilla.org/show_bug.cgi?id=1451744#c2

Comment 30

[Nicholas Nethercote [inactive]]

Thank you for backing out.

Comment 31

[Nicholas Nethercote [inactive]]

Bug 1449787 commit 2 explains the situation here.

Comment 32

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Since it looks like a modified part of this change set landed in bug 1449395, what will become of this bug? Will you attempt to land a modified patch set? What priority is this fix (do you intend it to land in 61 or later?)

Comment 33

[Nicholas Nethercote [inactive]]

This bug's patches will land when bug 1449787's land. The timing of that depends on a bugfix to MSVC, which is unpredictable. It's extremely unlikely to be in 61.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

What's the status of this now that we use clang on windows?

Comment 35

[Nicholas Nethercote [inactive]]

I recently talked with ajones about clang on Windows: MSVC is still in tier 2 support, and might be required for Windows/ARM. So we can't knowingly break MSVC builds yet. Maybe in Firefox 65.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/835ff9bb6d3f
Change nsRoleMapEntry::roleAtom from `nsStaticAtom**` to `nsStaticAtom* const`. r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/443109b897b8
Use `nsStaticAtom*` instead of `nsStaticAtom**` in nsAccessibilityService.h. r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf96b05721a4
Use `nsStaticAtom* const` instead of `nsStaticAtom**` in DocAccessible.cpp. r=davidb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b8bb97cf1d2
Use `nsStaticAtom*` instead of `nsStaticAtom**` in Element.h. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/972d5469e2b9
Reduce indirection for static atom pointers in nsCSSFrameConstructor.h. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/896cf4497b09
Reduce indirection for FrameConstructionDataByTag::mTag. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/876924556630
Reduce indirection for PseudoParentData::mPseudoType. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c157d088b4
Reduce indirection for static pointers in txCoreFunctionCall.cpp. r=erahm

Comment 37

[Nicholas Nethercote [inactive]]

The MSVC bug was fixed, and bug 1449787 is ready to land.

So these commits will increase the number of static constructors, but bug 1449787's commits will decrease them again.

Comment 38

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/835ff9bb6d3f
https://hg.mozilla.org/mozilla-central/rev/443109b897b8
https://hg.mozilla.org/mozilla-central/rev/cf96b05721a4
https://hg.mozilla.org/mozilla-central/rev/0b8bb97cf1d2
https://hg.mozilla.org/mozilla-central/rev/972d5469e2b9
https://hg.mozilla.org/mozilla-central/rev/896cf4497b09
https://hg.mozilla.org/mozilla-central/rev/876924556630
https://hg.mozilla.org/mozilla-central/rev/78c157d088b4