1261735 - Overhaul the atom implementation

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

The implementation of AtomImpl and PermanentAtomImpl in nsAtomTable.cpp is a bit ugly and can be improved.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Overhaul the atom implementation

Apologies for the hard-to-review patch. I couldn't break this up into smaller
pieces.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Inline some {Dynamic,Static}Atom methods

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - De-virtualize nsIAtom::IsStaticAtom()

This avoids the need for some virtual function calls and also will help lead to
distinct representations for dynamic and static atoms.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8737662 [details] [diff] [review]
(part 2) - Inline some {Dynamic,Static}Atom methods

+      buf = nsStringBuffer::Alloc((mLength + 1) * sizeof(char16_t));
+      mString = static_cast<char16_t*>(buf->Data());

Hmm, isn't nsStringBuffer::Alloc fallible?

Comment 5

[Nicholas Nethercote [inactive]]

> +      buf = nsStringBuffer::Alloc((mLength + 1) * sizeof(char16_t));
> +      mString = static_cast<char16_t*>(buf->Data());
> 
> Hmm, isn't nsStringBuffer::Alloc fallible?

Indeed, it is. I checked all the calls to that function. There's another unchecked call in nsHtml5Atom's constructor. It's a pre-existing problem so I'll fix it separately. I've file bug 1261744.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8737661 [details] [diff] [review]
(part 1) - Overhaul the atom implementation

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

r=me with a couple changes noted below.  Thanks for the comments.

::: xpcom/ds/nsAtomTable.cpp
@@ +30,5 @@
> +//   points to. |gAtomTable| holds weak references to them DynamicAtoms, and
> +//   when they are destroyed (due to their refcount reaching zero) they remove
> +//   themselves from |gAtomTable|.
> +//
> +// - Static: the atom itself is heap allocated, but it points to a static

Nit: given "DynamicAtom" in the item above, maybe this should be "StaticAtom"?

@@ +139,5 @@
> +  StaticAtom(nsStringBuffer* aData, uint32_t aLength, PLDHashNumber aKeyHash);
> +
> +  // This constructor must only be used in conjunction with placement new on an
> +  // existing DynamicAtom, in order to transmute that DynamicAtom into a
> +  // StaticAtom. The constructor does three notable things.

o/~ sketchy o/~

@@ +266,5 @@
> +  auto entry = static_cast<AtomTableEntry*>(aEntry);
> +  nsIAtom* atom = entry->mAtom;
> +  if (atom->IsStaticAtom()) {
> +    // This case only occurs when gAtomTable is destroyed, whereupon all
> +    // StaticAtoms within it must be explicitly deleted. The cast is required

Just for clarification, this code *can* get called when we're deleting DynamicAtoms from the table, so we can't simply MOZ_ASSERT staticness here, correct?

@@ +561,2 @@
>  
> +    MOZ_ASSERT(nsCRT::IsAscii((char16_t*)stringBuffer->Data()));

As long as you're touching lines in here, it would be splendid to convert the C-style casts to something C++-y.

@@ +571,5 @@
>      if (atom) {
> +      if (!atom->IsStaticAtom()) {
> +        // A rare case: we're creating a StaticAtom but there is already a
> +        // DynamicAtom of the same name. Transmute the StaticAtom into a
> +        // DynamicAtom. See the comment on the relevant StaticAtom constructor

I think the transmutation goes the other way, from DynamicAtom into StaticAtom.

::: xpcom/ds/nsIAtom.idl
@@ +72,5 @@
> +  // Any subclass of nsIAtom for which this function might be called must
> +  // implement it.
> +  virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) {
> +    MOZ_CRASH("Should not call this function");
> +  };

You could make this [noscript,notxpcom] in the idl itself and then every subclass would have to implement it, which might be a little nicer.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8737661 [details] [diff] [review]
(part 1) - Overhaul the atom implementation

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

f+ for now, I have a few questions below.

::: xpcom/ds/nsAtomTable.cpp
@@ +82,3 @@
>    // really refcounted.  But since these entries live in a global hashtable,
>    // this reference is essentially owning.
>    nsIAtom* MOZ_OWNING_REF mAtom;

I wonder why this isn't just a StaticAtom* if it only points to objects of type StaticAtom.

@@ +118,3 @@
>  
> +#ifdef DEBUG
> +static bool EqualStrings(char16_t* aStr1, char16_t* aStr2)

Why are you reimplementing strcmp?

@@ +147,5 @@
> +  //   the refcount.
> +  // - Releases the existing heap-allocated string buffer (explicitly),
> +  //   replacing it with the static string buffer (which must contain identical
> +  //   chars).
> +  explicit StaticAtom(nsStringBuffer* aStaticBuffer)

It would make me a lot happier if this were private. I liked the previous "Promote" wording, maybe add a:

>  static StaticAtom* StaticAtom::PromoteToStatic(DynamicAtom*)

This could do the in place new, and even static_assert that DynamicAtom is in fact large enough to in place new a StaticAtom over it.

@@ +152,3 @@
>    {
> +    char16_t* staticString = static_cast<char16_t*>(aStaticBuffer->Data());
> +    MOZ_ASSERT(EqualStrings(staticString, mString));

|nsCRT::strcmp(...) == 0| would suffice here.

@@ +153,5 @@
> +    char16_t* staticString = static_cast<char16_t*>(aStaticBuffer->Data());
> +    MOZ_ASSERT(EqualStrings(staticString, mString));
> +    nsStringBuffer* dynamicBuffer = nsStringBuffer::FromData(mString);
> +    mString = staticString;
> +    dynamicBuffer->Release();

I think this is okay, but I'm just going to write it out as we've diverged somewhat from the previous implementation.

Previously the "promote" was simply an in-place new, we didn't swap the buffers. Now we do, so we need to manually release the buffer. Is the swapping strictly necessary?

This is also missing the |NS_BUILD_REFCNT_LOGGING| shenanigans from |PromoteToPermanent|, which to be honest I don't fully follow but I suppose it's there for a reason.

Comment 8

[Nicholas Nethercote [inactive]]

> @@ +266,5 @@
> > +  auto entry = static_cast<AtomTableEntry*>(aEntry);
> > +  nsIAtom* atom = entry->mAtom;
> > +  if (atom->IsStaticAtom()) {
> > +    // This case only occurs when gAtomTable is destroyed, whereupon all
> > +    // StaticAtoms within it must be explicitly deleted. The cast is required
> 
> Just for clarification, this code *can* get called when we're deleting
> DynamicAtoms from the table, so we can't simply MOZ_ASSERT staticness here,
> correct?

If "this code" means "the function" -- yes. I put the comment inside the |if| to indicate that "This case" refers to static atoms only. I'll see if I can reword to make it clearer.

Comment 9

[Nicholas Nethercote [inactive]]

> ::: xpcom/ds/nsIAtom.idl
> @@ +72,5 @@
> > +  // Any subclass of nsIAtom for which this function might be called must
> > +  // implement it.
> > +  virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) {
> > +    MOZ_CRASH("Should not call this function");
> > +  };
> 
> You could make this [noscript,notxpcom] in the idl itself and then every
> subclass would have to implement it, which might be a little nicer.

I think I tried that and I had trouble with using mozilla::MallocSizeOf in IDL...

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> > ::: xpcom/ds/nsIAtom.idl
> > @@ +72,5 @@
> > > +  // Any subclass of nsIAtom for which this function might be called must
> > > +  // implement it.
> > > +  virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) {
> > > +    MOZ_CRASH("Should not call this function");
> > > +  };
> > 
> > You could make this [noscript,notxpcom] in the idl itself and then every
> > subclass would have to implement it, which might be a little nicer.
> 
> I think I tried that and I had trouble with using mozilla::MallocSizeOf in
> IDL...

Hm, looking at other instances something like:

native size_t(size_t)
native MallocSizeOf(mozilla::MallocSizeOf);

[noscript,notxpcom] size_t SizeOfIncludingThis(MallocSizeOf);

ought to work?

If some permutation on that doesn't work, how about just defining this as a pure virtual function, then?

Comment 11

[Nicholas Nethercote [inactive]]

> ::: xpcom/ds/nsAtomTable.cpp
> @@ +82,3 @@
> >    // really refcounted.  But since these entries live in a global hashtable,
> >    // this reference is essentially owning.
> >    nsIAtom* MOZ_OWNING_REF mAtom;
> 
> I wonder why this isn't just a StaticAtom* if it only points to objects of
> type StaticAtom.

Good question. Looks like it can be, but I have to move a bunch of code around so that things are declared in the appropriate order. I'll do that as a follow-up bug.


> Why are you reimplementing strcmp?

Ah... nsCRT::strcmp, that's where it is. I looked and couldn't find it and nobody on #developers was able to help me either. Thank you.


> @@ +147,5 @@
> > +  //   the refcount.
> > +  // - Releases the existing heap-allocated string buffer (explicitly),
> > +  //   replacing it with the static string buffer (which must contain identical
> > +  //   chars).
> > +  explicit StaticAtom(nsStringBuffer* aStaticBuffer)
> 
> It would make me a lot happier if this were private.
> I liked the previous "Promote" wording, maybe add a:
> 
> >  static StaticAtom* StaticAtom::PromoteToStatic(DynamicAtom*)
> 
> This could do the in place new,

I'll add something like that.


> and even static_assert that DynamicAtom is
> in fact large enough to in place new a StaticAtom over it.

Good idea.


> @@ +153,5 @@
> > +    char16_t* staticString = static_cast<char16_t*>(aStaticBuffer->Data());
> > +    MOZ_ASSERT(EqualStrings(staticString, mString));
> > +    nsStringBuffer* dynamicBuffer = nsStringBuffer::FromData(mString);
> > +    mString = staticString;
> > +    dynamicBuffer->Release();
> 
> I think this is okay, but I'm just going to write it out as we've diverged
> somewhat from the previous implementation.
> 
> Previously the "promote" was simply an in-place new, we didn't swap the
> buffers. Now we do, so we need to manually release the buffer. Is the
> swapping strictly necessary?

At the moment it isn't necessary. It's ok (though perhaps surprising) for a StaticAtom to point to a heap-allocated nsStringBuffer.

However, in the long run I hope to change StaticAtom so it can point to something other than an nsStringBuffer, because nsStringBuffer has a refcnt and a length, neither of which are useful for static atoms.


> This is also missing the |NS_BUILD_REFCNT_LOGGING| shenanigans from
> |PromoteToPermanent|, which to be honest I don't fully follow but I suppose
> it's there for a reason.

I also didn't fully follow it and I did a try push with it gone and there were no complaints :)

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Overhaul the atom implementation

New version addresses all the comments.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8738327 [details] [diff] [review]
(part 1) - Overhaul the atom implementation

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

Looks good. Just want to double-check, you did a try run of a debug build w/ trace ref counting enabled previously?

Comment 14

[Nicholas Nethercote [inactive]]

> Looks good. Just want to double-check, you did a try run of a debug build w/
> trace ref counting enabled previously?

Hmm, not with trace ref counting enabled. I'll try that.

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> > Looks good. Just want to double-check, you did a try run of a debug build w/
> > trace ref counting enabled previously?
> 
> Hmm, not with trace ref counting enabled. I'll try that.

Oh, it's enabled by default on debug builds. So I have tried it. I'll do another try run before landing, too, since there were quite a few changes.

Thank you both for the good review comments.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Change StaticAtomEntry::mAtom to |StaticAtom*|

The diff is a nightmare. I promise that the only change I made other than
reordering things and adding some "//----" separator comments was the type
change for StaticAtomEntry::mAtom :)

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06d9d9cd6ab1187c284a46aa6830aa963d86456d
Bug 1261735 (part 1) - Overhaul the atom implementation. r=froydnj,erahm.

Comment 18

[Eric Rahm [:erahm]]

Comment on attachment 8738353 [details] [diff] [review]
(part 4) - Change StaticAtomEntry::mAtom to |StaticAtom*|

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

Thanks for following up.

::: xpcom/ds/nsAtomTable.cpp
@@ +451,5 @@
> +  enum { ALLOW_MEMMOVE = true };
> +
> +  // mAtom only points to objects of type StaticAtom, which are not
> +  // really refcounted.  But since these entries live in a global hashtable,
> +  // this reference is essentially owning.

This comment could probably be tweaked now.

Comment 19

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/492d49a7e4f06587f470c78f4411957f9e4fb4d0
Bug 1261735 (part 2) - Inline some {Dynamic,Static}Atom methods. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/916383c80fc7ebf99ff3eee28aad272a81c4682f
Bug 1261735 (part 3) - De-virtualize nsIAtom::IsStaticAtom(). r=froydnj,erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e21ac5e6a8b8364c769867c2118efa9c8e89134
Bug 1261735 (part 4) - Change StaticAtomEntry::mAtom to |StaticAtom*|. r=erahm.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06d9d9cd6ab1
https://hg.mozilla.org/mozilla-central/rev/492d49a7e4f0
https://hg.mozilla.org/mozilla-central/rev/916383c80fc7
https://hg.mozilla.org/mozilla-central/rev/3e21ac5e6a8b