Closed Bug 1500362 Opened 6 years ago Closed 6 years ago

represent static atoms in style system code using an index into the static atom table, rather than a pointer

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files, 3 obsolete files)

For bug 1474793, I'll need a static representation for static atoms, since the nsStaticAtom* pointer values are not known until run time. The patches here will add a handle value to nsStaticAtom (which is just its index into the nsGkAtoms static atom table), and change the Atom type in style system code to use that handle value rather than the nsAtom*. On 64 bit we opportunistically store the static atom hash in that handle value too, so that we don't have to read out the hash from the nsAtom. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb274a68e91a645b6302c83e23dd8c5fba8c318
This will be used in the next patch to switch the style system's canonical representation for atoms to be either a static atom handle value or a dynamic atom pointer. MozReview-Commit-ID: 2F5xhtwKTgh
This will allow static atom values to be used cross-process without needing to translate from one process's atom pointer value to another's. MozReview-Commit-ID: AEPoICQi2sM Depends on D9233
> For bug 1474793, I'll need a static representation for static atoms, since the nsStaticAtom* pointer values are not known until run time. Am I misunderstanding? AFAIK this sentence is incorrect. Static atoms have had a static representation for some time now.
This works, and I think it's a bit less intrusive, plus it doesn't require branching for Atom::deref. I moved the gGkAtoms thing outside of the mozilla::detail namespace because I figured it was slightly better, though I can see the argument against it as well. I can undo that change and go back at cfg_if plus figuring out the right symbols, or... Actually bindgen can generate the right thing already, it's just an oversight the fact that it outputs `static mut` instead of plain `static`. An alternative path forward (maybe preferable to avoid exposing the un-namespaced `gGkAtoms`) is to fix bindgen, update it in Gecko, and use it to get the gGkAtoms symbol with the right qualification. I've submitted a fix here: https://github.com/rust-lang-nursery/rust-bindgen/pull/1427 So should be possible as well. Just let me know if you want to wait for that to land this or not.
(In reply to Nicholas Nethercote [:njn] from comment #3) > > For bug 1474793, I'll need a static representation for static atoms, since the nsStaticAtom* pointer values are not known until run time. > > Am I misunderstanding? AFAIK this sentence is incorrect. Static atoms have > had a static representation for some time now. I mean, a static representation that is the identical in all processes. Per some IRL discussion previously, I think we need to go with something like my patches rather than the comment 4 patch, since otherwise we won't have comparable static atom values across processes. After a few days of struggling, today's rebase and try push of my patches seem much happier on Talos for whatever reason: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f8dd37830b4e&newProject=try&newRevision=32eb5cb3d544&framework=1&showOnlyImportant=1 The tsvr_opacity regression isn't really a regression. Checking the graph over the past 30 days, this test seems to be bi-modal, and the test runs with my patches applied happen to fall on the higher of the two times.
Attachment #9018570 - Attachment is obsolete: true
Attachment #9018571 - Attachment is obsolete: true
Attachment #9018794 - Attachment is obsolete: true
Depends on: 1517685
Depends on D15078
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f4151dfd56e5 Use atom handles in favour of atom pointers in style system code r=emilio https://hg.mozilla.org/integration/autoland/rev/b7e4f2bbfe57 Make GkAtoms opaque to avoid lld-link.exe errors r=emilio
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
No longer depends on: 1517685
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: