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

RESOLVED DUPLICATE of bug 1362338

Status

()

RESOLVED DUPLICATE of bug 1362338
2 years ago
2 years ago

People

(Reporter: emilio, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
Flags: needinfo?(nfroyd)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1362338
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?
Flags: needinfo?(nfroyd)
(Reporter)

Comment 4

2 years ago
(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.
You need to log in before you can comment on or make changes to this bug.