Closed Bug 1451169 Opened 2 years ago Closed 1 year ago

Improve static atom handling in various places

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(8 files)

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 on attachment 8964760 [details]
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in Element.h.

https://reviewboard.mozilla.org/r/233486/#review239084
Attachment #8964760 - Flags: review?(amarchesini) → review+
Comment on attachment 8964761 [details]
Bug 1451169 - Reduce indirection for static atom pointers in nsCSSFrameConstructor.h.

https://reviewboard.mozilla.org/r/233488/#review239132
Attachment #8964761 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8964762 [details]
Bug 1451169 - Reduce indirection for FrameConstructionDataByTag::mTag.

https://reviewboard.mozilla.org/r/233490/#review239144
Attachment #8964762 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8964763 [details]
Bug 1451169 - Reduce indirection for PseudoParentData::mPseudoType.

https://reviewboard.mozilla.org/r/233492/#review239146
Attachment #8964763 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8964757 [details]
Bug 1451169 - Change nsRoleMapEntry::roleAtom from `nsStaticAtom**` to `nsStaticAtom* const`.

https://reviewboard.mozilla.org/r/233480/#review239286
Attachment #8964757 - Flags: review?(dbolter) → review+
Comment on attachment 8964758 [details]
Bug 1451169 - Use `nsStaticAtom*` instead of `nsStaticAtom**` in nsAccessibilityService.h.

https://reviewboard.mozilla.org/r/233482/#review239288
Attachment #8964758 - Flags: review?(dbolter) → review+
Comment on attachment 8964759 [details]
Bug 1451169 - Use `nsStaticAtom* const` instead of `nsStaticAtom**` in DocAccessible.cpp.

https://reviewboard.mozilla.org/r/233484/#review239290
Attachment #8964759 - Flags: review?(dbolter) → review+
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?
Attachment #8964764 - Flags: review?(erahm) → review+
> 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).
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
Depends on: 1451744
Backout by aciure@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/d1db4d5030c8
Backed out 8 changesets on request from njn a=backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Backed out as requested https://bugzilla.mozilla.org/show_bug.cgi?id=1451744#c2
Flags: needinfo?(n.nethercote)
Thank you for backing out.
Flags: needinfo?(n.nethercote)
Bug 1449787 commit 2 explains the situation here.
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?)
Flags: needinfo?(n.nethercote)
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.
Flags: needinfo?(n.nethercote)
Component: General → XPCOM
What's the status of this now that we use clang on windows?
Flags: needinfo?(n.nethercote)
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.
Flags: needinfo?(n.nethercote)
Pushed by nnethercote@mozilla.com:
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
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.
You need to log in before you can comment on or make changes to this bug.