1445196 - Store whether an atom is pinned in the atom itself

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Attachment: pinned-atom-flag

Currently we store this state in the atoms table entry, which requires a lookup in a couple of places, notable AtomIsPinnedInRuntime which is used in tracing assertions.  This complicates tracing helper threads because it requires the exclusive access lock and we have to take care to acquire the locks in the right order.

We can store this state in the atoms flags word.  Here's an initial patch.

This keeps the state in the atoms table entry as well so it doesn't require touching all the atoms during marking, but I'm not sure how important this is.  I'm a bit wary of doing anything that could affect marking speed because the atoms table can be pretty large.

We could go further and set the pinned flag for static strings and permanent atoms which would further simplify AtomIsPinned but I haven't done that yet.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #0)
> We could go further and set the pinned flag for static strings and permanent
> atoms which would further simplify AtomIsPinned but I haven't done that yet.

I think we should do this to avoid races (the flags word) when pinning a permanent atom, because permanent atoms are shared with worker runtimes?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #1)

It looks like we treat permanent atoms as pinned in AtomizeString() regardless, and don't set the flag.  But I still think this is worth doing.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8958391 [details] [diff] [review]
pinned-atom-flag

Review of attachment 8958391 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/vm/JSAtom.cpp
@@ +245,5 @@
>      emptyString = nullptr;
>  }
>  
>  static inline void
>  TracePinnedAtoms(JSTracer* trc, const AtomSet& atoms)

If most atoms are not pinned it might make sense to have pinned vs non-pinned sets?

@@ +249,5 @@
>  TracePinnedAtoms(JSTracer* trc, const AtomSet& atoms)
>  {
>      for (auto r = atoms.all(); !r.empty(); r.popFront()) {
>          const AtomStateEntry& entry = r.front();
> +        MOZ_ASSERT(entry.isPinned() == entry.asPtrUnbarriered()->isPinned());

Great assert.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: pinned-atom-flag-2

Part 2 - mark permanent atoms and static strings as pinned and use isPinned() to simplify checks.

I changed the JSAtom version of ThingIsPermanent() to check for pinned atoms rather than just permanent atoms so the name is less fitting now (but it didn't fit for symbols previously either).  Perhaps I should think of something better.

Comment 5

[Tom S. (please needinfo tschuster)]

Comment on attachment 8958391 [details] [diff] [review]
pinned-atom-flag

Review of attachment 8958391 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/StringType.h
@@ +1165,5 @@
>      }
>  
> +    MOZ_ALWAYS_INLINE
> +    bool isPinned() const {
> +        return d.u1.flags & PINNED_ATOM_BIT;

Assert isAtom()?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8958496 [details] [diff] [review]
pinned-atom-flag-2

Review of attachment 8958496 [details] [diff] [review]:
-----------------------------------------------------------------

This is much simpler.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8958496 [details] [diff] [review]
pinned-atom-flag-2

Review of attachment 8958496 [details] [diff] [review]:
-----------------------------------------------------------------

Oh btw shouldn't we also check isPinned in ThingIsPermanentAtomOrWellKnownSymbol?

I'm wondering if we can remove the permanent-atom bit and check isPinned everywhere; it's a very similar concept.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/471a52bfd1ee
Store the atom's pinned flag in the atom to simplify lookup r=jandem

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/471a52bfd1ee

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Oh btw shouldn't we also check isPinned in ThingIsPermanentAtomOrWellKnownSymbol?
> 
> I'm wondering if we can remove the permanent-atom bit and check isPinned
> everywhere; it's a very similar concept.

I had a look at this and realised that permanent atoms are a different concept because they are shared between all runtimes in a process and we need to be able to distinguish these because we don't want to mark cells that are owned by a different runtime.