1387992 - stylo: Consider making DynamicAtom public so we can devirtualize AddRefing/Relasing them.

Status: RESOLVED DUPLICATE of bug 1362338

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

Right now this is hot on stylist rebuild times because the Rust's hashmap entry API forces an unconditional clone of the key.

Most of the atoms in stylists are dynamic. I wonder if we could make it public so  addrefing/releasing an atom is a function call, not an FFI call that calls a virtual method.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Nathan, does it sound reasonable to make DynamicAtom public? We could also entirely devirtualize nsIAtom::AddRef/Release, I guess, though that's harder because it inherits from nsISupports. But marking it |final| the compiler should be able to figure it out, I guess.

The basic idea would be something like what stylo does already:

nsrefcnt
nsIAtom::AddRef()
{
  if (mIsStatic) {
    return 2;
  }
  return static_cast<DynamicAtom*>(this)->AddRef();
}

Where DynamicAtom::AddRef is an inline function. Similarly for Release(). This would avoid us exposing DynamicAtom around.

Comment 3

[Nathan Froyd [:froydnj]]

Xidorn already dup'd this, but I am confused about "something like what stylo does already"...does stylo glue code try to devirtualize things on its own?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Xidorn already dup'd this, but I am confused about "something like what
> stylo does already"...does stylo glue code try to devirtualize things on its
> own?

To some extent. We already avoid going through the FFI boundary if mIsStatic is non-zero.