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

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 months ago
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
(Assignee)

Comment 1

7 months ago
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
(Assignee)

Comment 2

7 months ago
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.
(Assignee)

Comment 5

6 months ago
(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
(Assignee)

Updated

4 months ago
Depends on: 1517685

Comment 9

4 months ago
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

Comment 10

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
(Assignee)

Updated

4 months ago
No longer depends on: 1517685
You need to log in before you can comment on or make changes to this bug.