1362338 - Consider devirtualizing refcounting of nsIAtom

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P4)

Description

[Xidorn Quan [:xidorn] UTC+10]

For having fewer string conversion between servo and gecko, we are converting many places from nsString to nsIAtom.

Comparing to refcounting of nsStringBuffer, nsIAtom has an additional virtual call for refcounting, which is probably avoidable.

nsIAtom only has three concrete subclasses: StaticAtom, DynamicAtom, and HTML5Atom. StaticAtom is distinguishable by mIsStatic field. We can extend that field to two bits to make all three distinguishable from each other, then we can devirtualize AddRef and Release.

(Actually HTML5Atom is private in HTML5AtomTable, and it doesn't actually do any refcounting, so I wonder whether we can do something clever here... but taking one more bit from mLength doesn't seem to be a bit deal anyway, unless we ever want to atomize a string larger than 1GB...)

We would also need to replace nsCOMPtr<nsIAtom> with RefPtr<nsIAtom> to take advantage of the devirtualization.

Comment 1

[Nathan Froyd [:froydnj]]

I would be completely on board with this, even to the point of writing it.  The only wrinkle is that nsIAtom is scriptable, and I don't know if we create atoms from script and depend on that, or if addons create atoms from script....

I guess we could try implementing a C++-only Atom class, and then somehow make the bits we need to expose to script depend on that?  Then the C++ bits can use non-virtual refcounting, and only the script bits are paying the virtual function overhead.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Yeah, we can probably add a mozilla::Atom which implements nsIAtom and replace all usages in C++ and Rust with it...

What happens to the C++ fields when someone derive nsIAtom from script? Is there any function which accepts an nsIAtom from script? That sounds pretty dangerous...

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Ah, wait, nsIAtom is builtinclass so script cannot implement it, only C++ can, and anyone wants to create it from script would need to call into some function implemented in C++, e.g. nsIAtomService.getAtom. So I don't think we have issue there.

Comment 4

[Nathan Froyd [:froydnj]]

There are a *ridiculous* number of addons that touch nsIAtomService, even though Gecko itself does not. :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm not entirely sure I follow how we plan to devirtualize.  nsIAtom inherits from nsISupports, right?  nsISupports has virtual AddRef/Release methods.  Any methods with those names that we define on nsIAtom will become virtual, as far as I can tell...

Or is the idea to mark nsIAtom's AddRef/Release as final and assume the compiler devirtualizes those?  If that's the plan, then I don't think we need to worry about script bits; those will just refcount via the virtual nsISupports methods, which should do the right thing.  And as noted script can only pass atoms around; it can't _implement_ them.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> There are a *ridiculous* number of addons that touch nsIAtomService

Oh, and that's because we have or used to have APIs that require nsIAtom.  Most likely the main culprit is nsITreeView, which is expected to be implemented in JS and used to have methods that were expected to fill in a passed-in nsISupportsArray with atoms, until we landed bug 407956.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> Or is the idea to mark nsIAtom's AddRef/Release as final and assume the
> compiler devirtualizes those?  If that's the plan, then I don't think we
> need to worry about script bits; those will just refcount via the virtual
> nsISupports methods, which should do the right thing.  And as noted script
> can only pass atoms around; it can't _implement_ them.

Yeah, that's my plan, but froydnj seems to be concerned about that implementing virtual functions in nsIAtom may cause some compilers to reorder items in vtable, which can affect xpconnect.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> I'm not entirely sure I follow how we plan to devirtualize.  nsIAtom
> inherits from nsISupports, right?  nsISupports has virtual AddRef/Release
> methods.  Any methods with those names that we define on nsIAtom will become
> virtual, as far as I can tell...

My plan was to have something like:

// Not exactly sure how this would work, because the refcounting methods
// for DynamicAtom and StaticAtom are completely different.  Maybe would
// have to lift AddRef/Release to Atom itself and have it switch depending
// on the kind of Atom.
class Atom { ... };
class DynamicAtom final : public Atom { ... };
class StaticAtom final : public Atom { ... };

class ScriptableAtom : public nsIAtom
{
  RefPtr<Atom> mAtom; // :p
};

Not actually sure that would survive contact with the mass of Gecko code that uses nsIAtom...

> Or is the idea to mark nsIAtom's AddRef/Release as final and assume the
> compiler devirtualizes those?  If that's the plan, then I don't think we
> need to worry about script bits; those will just refcount via the virtual
> nsISupports methods, which should do the right thing.  And as noted script
> can only pass atoms around; it can't _implement_ them.

This was Xidorn's idea, and would be much easier, assuming that the compiler will actually devirtualize in all the cases we care about.  I can try this, 'cause it should be about 20 lines of code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I don't understand how the reordering could happen.  We're talking about virtual functions that nsIAtom inherits from nsISupports.  Their placement in the vtable has to match the nsISupports placement; otherwise accessing them via an nsISupports* would not work, right?

I suspect trying to boil the "don't use nsIAtom" ocean is a much larger task.

Comment 10

[Benjamin Smedberg]

I don't think that the c++ Atom should inherit from nsISupports.

We could still have a scriptable nsIAtom for the stupid treeview interface.

FWIW, we shouldn't make addon-breaking changes to nsIAtom until 57, and then please break it at will.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> We could still have a scriptable nsIAtom for the stupid treeview interface.

To be clear, treeview no longer uses nsIAtom and hasn't for a few years.

Comment 12

[Nathan Froyd [:froydnj]]

Whatever we do here will be much easier after bug 1269490 lands, so we don't have atoms from the HTML parser to deal with.

Comment 13

[Bobby Holley (:bholley)]

Atom overhead in style is significant. We should get this landed if we can.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Now that bug 1269490 has landed, should we look into this bug again?

It seems bug 1269490 only merges static atoms used in HTML parser into nsGkAtoms, and they still have an independent nsHTML5Atom subclass of nsIAtom which is neither static not refcounted. So it seems to me that the situation actually doesn't change much by that bug.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> Now that bug 1269490 has landed, should we look into this bug again?
> 
> It seems bug 1269490 only merges static atoms used in HTML parser into
> nsGkAtoms, and they still have an independent nsHTML5Atom subclass of
> nsIAtom which is neither static not refcounted. So it seems to me that the
> situation actually doesn't change much by that bug.

Yeah, but I think devirtualizing AddRef/Release is not impossible.

We could use another bit in nsIAtom in order to mark nsHTML5Atoms (reducing nsIAtom's max length to 2^30 - 1). Or maybe we could take that bit from the hash? idk.

Another option is moving DynamicAtom to a header file, inlining AddRef/Release. That allows us to get rid of the virtual call on the Rust -> C++ paths from stylo, where we can't have nsHTML5Atom (only StaticAtom or DynamicAtom).

Nathan, which of the two solutions would you find preferrable? I've seen these on profiles quite a lot of times.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> > Now that bug 1269490 has landed, should we look into this bug again?
> > 
> > It seems bug 1269490 only merges static atoms used in HTML parser into
> > nsGkAtoms, and they still have an independent nsHTML5Atom subclass of
> > nsIAtom which is neither static not refcounted. So it seems to me that the
> > situation actually doesn't change much by that bug.
>
> We could use another bit in nsIAtom in order to mark nsHTML5Atoms (reducing
> nsIAtom's max length to 2^30 - 1). Or maybe we could take that bit from the
> hash? idk.

I think I'd prefer to take a bit from the length rather than the hash...but do we really need another bit?  nsHtml5Atom has the same AddRef/Release scheme as StaticAtom (its lifetime is managed more like an UniquePtr-style class, rather than through AddRef/Release), so I think we can get by with the single bit we have today.

The intent here is that this would enable devirtualization by giving nsIAtom an AddRef/Release that would be suitable for all existing subclasses, and then marking AddRef/Release on nsIAtom final?

> Nathan, which of the two solutions would you find preferrable? I've seen
> these on profiles quite a lot of times.

I think I would prefer the first scheme--assuming I'm understanding it correctly and our compilers DTRT--rather than publicizing DynamicAtom.  It has a higher chance of benefiting all of Gecko, rather than just the Rust-specific call paths, and will involve less casting and whatnot.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> I think I'd prefer to take a bit from the length rather than the hash...but
> do we really need another bit?  nsHtml5Atom has the same AddRef/Release
> scheme as StaticAtom (its lifetime is managed more like an UniquePtr-style
> class, rather than through AddRef/Release), so I think we can get by with
> the single bit we have today.

We do, because nsHTML5Atom doesn't flag itself as mIsStatic, and it relies on that.

I'll take another one from the length.

> The intent here is that this would enable devirtualization by giving nsIAtom
> an AddRef/Release that would be suitable for all existing subclasses, and
> then marking AddRef/Release on nsIAtom final?

Right, that'd be the intent of the first option.

> I think I would prefer the first scheme--assuming I'm understanding it correctly and our compilers DTRT--rather than publicizing DynamicAtom.  It has a higher chance of benefiting all of Gecko, rather than just the Rust-specific call paths, and will involve less casting and whatnot.

Agreed, will do it this way then.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1362338: Manually whitelist nsIAtom::Release in the root analysis.

Review commit: https://reviewboard.mozilla.org/r/166094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166094/

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1362338: Take a bit from the atom's length to store a tristate.

Review commit: https://reviewboard.mozilla.org/r/166096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166096/

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1362338: Make nsIAtom::AddRef and nsIAtom::Release final.

I can put a bit more effort in deduplicating the QueryInterface / mRefCnt
declarations if needed.

Review commit: https://reviewboard.mozilla.org/r/166098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166098/

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Disclaimer, the patches over here require https://github.com/servo/servo/pull/18014 to not crash stylo.

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8894977 [details]
Bug 1362338: Manually whitelist nsIAtom::Release in the root analysis.

https://reviewboard.mozilla.org/r/166094/#review171310

+1 for whitespace changes in a separate commit!

::: commit-message-3138e:1
(Diff revision 1)
> +Bug 1362338: Preliminar whitespace cleanup in nsIAtom.idl. r?froydnj

Nit: "Preliminary"

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 8894978 [details]
Bug 1362338: Take a bit from the atom's length to store a tristate.

https://reviewboard.mozilla.org/r/166096/#review171312

::: xpcom/ds/nsIAtom.idl:104
(Diff revision 1)
> -  uint32_t mLength:31;
> -  uint32_t mIsStatic:1;
> +  uint32_t mLength: 30;
> +  uint32_t mKind: 2; // nsIAtom::Kind

Kinda wonder if it's worth splitting this into two 16-bit fields, so accessing things gets a bit cheaper (64K-character atoms should be enough for anybody, right?).

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 8894979 [details]
Bug 1362338: Make nsIAtom::AddRef and nsIAtom::Release final.

https://reviewboard.mozilla.org/r/166098/#review171316

Yay!  I guess you haven't really modified any places that would take advantage of the devirtualizable methods...do you have a patch and have you checked that the methods are devirtualized as you would expect?

::: parser/html/nsHtml5Atom.h:21
(Diff revision 1)
>   * Usage is documented in nsHtml5AtomTable and nsIAtom.
>   */
>  class nsHtml5Atom final : public nsIAtom
>  {
>    public:
> -    NS_DECL_ISUPPORTS
> +    NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) final;

Nice, `nsHtml5Atom` gets a little smaller with these changes, too, since we don't have to declare an unnecessary refcount.

::: xpcom/ds/nsAtomTable.cpp:139
(Diff revision 1)
> +  ThreadSafeAutoRefCnt mRefCnt;
> +  NS_DECL_OWNINGTHREAD

Bleh, it's unfortunate that we have to pay for the owningthread stuff on threadsafe refcounted classes as well.  (Not your fault, of course.)

::: xpcom/ds/nsAtomTable.cpp:283
(Diff revision 1)
> +MozExternalRefCountType
> +nsIAtom::AddRef()
> +{
> +  MOZ_ASSERT(!IsHTML5Atom(), "Attempt to AddRef an nsHtml5Atom");
> +  if (!IsDynamicAtom()) {
> +    return 2;

Maybe `MOZ_ASSERT(IsStaticAtom())` here, so people will fail loudly if they try adding a new atom type?

::: xpcom/ds/nsAtomTable.cpp:293
(Diff revision 1)
> +MozExternalRefCountType
> +nsIAtom::Release()
> +{
> +  MOZ_ASSERT(!IsHTML5Atom(), "Attempt to Release an nsHtml5Atom");
> +  if (!IsDynamicAtom()) {
> +    return 1;

Likewise here.

::: xpcom/ds/nsIAtom.idl:98
(Diff revision 1)
> +  MozExternalRefCountType AddRef() final;
> +  MozExternalRefCountType Release() final;

Please make sure to run tests that exercise getting atoms from JS (if we have any...); MSVC in particular has a habit of reordering virtual function tables.  I don't remember all the rules offhand, and I *think* declaring these here shouldn't be a problem, but double-checking would be good.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894979 [details]
Bug 1362338: Make nsIAtom::AddRef and nsIAtom::Release final.

https://reviewboard.mozilla.org/r/166098/#review171316

The two callsites I care about the most right now (`Gecko_AddRefAtom` and `Gecko_ReleaseAtom`) should get that benefit for free (they call `nsIAtom::AddRef` directly), and I can confirm that they appear higher up in profiles with this last pat reverted.

I think for most users to take advantage of this we'd want to fix bug 1363754... I should probably get back to it and take proper measurements.

> Likewise here.

Good call

> Please make sure to run tests that exercise getting atoms from JS (if we have any...); MSVC in particular has a habit of reordering virtual function tables.  I don't remember all the rules offhand, and I *think* declaring these here shouldn't be a problem, but double-checking would be good.

I don't think we have any JS-implemented atoms in tree, but I'll take a closer look just in case.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894977 [details]
Bug 1362338: Manually whitelist nsIAtom::Release in the root analysis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166094/diff/1-2/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894978 [details]
Bug 1362338: Take a bit from the atom's length to store a tristate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166096/diff/1-2/

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894979 [details]
Bug 1362338: Make nsIAtom::AddRef and nsIAtom::Release final.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166098/diff/1-2/

Comment 30

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 9 changes to 5 files
remote: (6037949963aa modifies servo/components/style/gecko_string_cache/mod.rs from Servo; not enforcing peer review)
remote: (1 changesets contain changes to protected servo/ directory: 6037949963aa)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote

Comment 31

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/18019

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894977 [details]
Bug 1362338: Manually whitelist nsIAtom::Release in the root analysis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166094/diff/2-3/

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894978 [details]
Bug 1362338: Take a bit from the atom's length to store a tristate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166096/diff/2-3/

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894979 [details]
Bug 1362338: Make nsIAtom::AddRef and nsIAtom::Release final.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166098/diff/2-3/

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea53d932b583
Preliminary whitespace cleanup in nsIAtom.idl. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/eb51672d2e2f
Take a bit from the atom's length to store a tristate. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/2980183d98fb
Make nsIAtom::AddRef and nsIAtom::Release final. r=froydnj

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19b8d672b55d
followup: Tentatively fix windows build bustage. r=me

Comment 37

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8808e9b8faf8
Backed out changesets 19b8d672b55d and 2980183d98fb for hazard analysis failures.

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894979 [details]
Bug 1362338: Make nsIAtom::AddRef and nsIAtom::Release final.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166098/diff/3-4/

Comment 39

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894977 [details]
Bug 1362338: Manually whitelist nsIAtom::Release in the root analysis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166094/diff/3-4/

Comment 40

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8894977 [details]
Bug 1362338: Manually whitelist nsIAtom::Release in the root analysis.

https://reviewboard.mozilla.org/r/166094/#review172340

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54888c1fde2a
Make nsIAtom::AddRef and nsIAtom::Release final. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9f7b94f19003
Manually whitelist nsIAtom::Release in the root analysis. r=froydnj,sfink

Comment 42

[Emilio Cobos Álvarez (:emilio)]

(In reply to Pulsebot from comment #41)
> Manually whitelist nsIAtom::Release in the root analysis. r=froydnj,sfink

I don't know why autoland thought this was r=froydnj, oh well...

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea53d932b583
https://hg.mozilla.org/mozilla-central/rev/eb51672d2e2f

Comment 44

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/54888c1fde2a
https://hg.mozilla.org/mozilla-central/rev/9f7b94f19003