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

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

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

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1500362 - Part 1: Store a handle value in nsStaticAtom objects.

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

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1500362 - Part 2: Use atom handles in favour of atom pointers in style system code.

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

Comment 3

[Nicholas Nethercote [inactive]]

> 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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1500362 - Allow Rust to construct static atoms, and use them for media features.

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.

Comment 5

[Cameron McCormack (:heycam)]

(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.

Comment 6

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=506859eccc16d6ee7b04c442e81ad13d8e701f2d&newProject=try&newRevision=ac1b9378bbabc154bc6b1558ecda335f0e67b32d&framework=1

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1500362 - Use atom handles in favour of atom pointers in style system code

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1500362 - Make GkAtoms opaque to avoid lld-link.exe errors

Depends on D15078

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f4151dfd56e5
https://hg.mozilla.org/mozilla-central/rev/b7e4f2bbfe57