PSA: Supplementable is going away

[Steinar H. Gunderson]

Hi all,

We're making a change in Supplementable<T> (and Supplement<T>) to remove it;
we already changed it from a hash table to a simple array underneath,
with good size gains (and a small Speedometer3 gain from the single hottest
member).

For members where there is no layering violation, we'll simply
forward-declare the class in the .h file and have a Member<>,
along with a getter and setter.

For members where we cannot do that (e.g. Document, in, core/document.h,
wants to have a Member<RTCPeerConnectionController>, which is defined
in modules/peerconnection/rtc_peer_connection_controller.h, and core/
cannot #include anything from modules/) we'll instead use the new
ForwardDeclaredMember<> which has almost the same ergonomics but
under-the-hood Trace()s via a vtable pointer like Supplement<> does today.

There's going to be a bunch of new getters and setters (and Members)
showing up, but they already existed through template magic; they are
just becoming visible now and given names and types (as opposed to being
string-keyed, which risks type confusion). We expect a tiny further
decrease in binary size and no significant change in compilation time.

Example CL:
https://chromium-review.googlesource.com/c/chromium/src/+/7137540

/* Steinar */
--
Homepage: https://www.sesse.net/

[Jeremy Roman]

Curious about the motivation here, beyond switching from a hashtable to an array. Is the tracing cost that large?

It seems like it forces a lot more boilerplate in very central classes and may make some of them considerably larger because they need space for members, some of which are nearly always absent. Especially across the core/ and modules/ divide, we'll need to forward-declare code that this class doesn't need to know about, which is itself a layering violation albeit one that doesn't break the compile.

We have a similar concept outside Blink, in the form of base::SupportsUserData, for similar reasons (and we presumably wouldn't want WebContentsImpl to have members for every possible user data it might have.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/ezwpu2bqbv7zau2uzqesynuaxpi7irjg2u2ojsshuktmjjkg4u%40oy2xauimoxhn.

[Steinar H. Gunderson]

On Thu, Nov 27, 2025 at 03:46:37PM -0500, Jeremy Roman wrote:
> Curious about the motivation here, beyond switching from a hashtable to an
> array. Is the tracing cost that large?

We won ~70 kB of APK size, and 2%+ style performance on some pages. The
tracing cost should be roughly the same. (The project was finished last
week, and Supplementable is gone.)

> It seems like it forces a lot more boilerplate in very central classes and
> may make some of them considerably larger because they need space for
> members, some of which are nearly always absent.

Are you asking about the change from a hashtable to an array, or the change
from an array to explicit members?

The size (which changed from hashtable to array) diff was generally small,
and often negative. The hash table was not free, even less so if you had
1–2 members set and needed to go to a heap allocation. In any case, unused
padding in these objects is probably much more significant if we care about
the RAM usage in the 2–3 Document objects we have in a typical renderer
process.

The boilerplate (which changed from array to explicit members) the same as
every other getter and setter, so normal Google/Chromium/Blink style.
I don't think there's been a push for trying to e.g. add macros to reduce
the boilerplate for a typical class, or for that matter, making the members
public? I mean, Supplementable comes purely from a desire to be able to embed
classes across layers, right?

> We have a similar concept outside Blink, in the form of
> base::SupportsUserData, for similar reasons (and we presumably wouldn't
> want WebContentsImpl to have members for every possible user data it might
> have.

There are valid reasons for having a sparse structure; in particular,
there are things like ElementRareData or NodeData where it genuinely saves
memory (and has seen different implementations as we've been tuning things).
But we have _lots_ of Elements, so the tradeoff is very different IMO.

[Jeremy Roman]

I guess I'm not sure I'm asking anything be done now that this is done, but I'm a bit surprised we unwound a longstanding technical decision so suddenly.

On Thu, Nov 27, 2025 at 4:19 PM Steinar H. Gunderson <se...@chromium.org> wrote:

On Thu, Nov 27, 2025 at 03:46:37PM -0500, Jeremy Roman wrote:
> Curious about the motivation here, beyond switching from a hashtable to an
> array. Is the tracing cost that large?

We won ~70 kB of APK size, and 2%+ style performance on some pages. The
tracing cost should be roughly the same. (The project was finished last
week, and Supplementable is gone.)

> It seems like it forces a lot more boilerplate in very central classes and
> may make some of them considerably larger because they need space for
> members, some of which are nearly always absent.

Are you asking about the change from a hashtable to an array, or the change
from an array to explicit members?

Explicit members.

 

The size (which changed from hashtable to array) diff was generally small,
and often negative. The hash table was not free, even less so if you had
1–2 members set and needed to go to a heap allocation. In any case, unused
padding in these objects is probably much more significant if we care about
the RAM usage in the 2–3 Document objects we have in a typical renderer
process.

The boilerplate (which changed from array to explicit members) the same as
every other getter and setter, so normal Google/Chromium/Blink style.
I don't think there's been a push for trying to e.g. add macros to reduce
the boilerplate for a typical class, or for that matter, making the members
public? I mean, Supplementable comes purely from a desire to be able to embed
classes across layers, right?

My question isn't about the particular way it's written (it's idiomatic, I have no doubt), just about core class headers being less readable because they need to contain potentially a lot (maybe it's less bad than in practice than I'd imagined) of code that isn't useful in understanding the class itself.

ExecutionContext now has dozens of these simply because it's the scope of most "singletons" in the web that aren't specifically document-scoped. Several of them are even platform-specific in a class that's generally very platform-agnostic (for example, WebViewAndroid and CrosKiosk), as well as several that seem a bit mysterious without context from the module they belong to.

> We have a similar concept outside Blink, in the form of
> base::SupportsUserData, for similar reasons (and we presumably wouldn't
> want WebContentsImpl to have members for every possible user data it might
> have.

There are valid reasons for having a sparse structure; in particular,
there are things like ElementRareData or NodeData where it genuinely saves
memory (and has seen different implementations as we've been tuning things).
But we have _lots_ of Elements, so the tradeoff is very different IMO.

content::WebContentsImpl is an example where we have fewer than we have blink::Elements, but certainly agreed that the tradeoffs are different.

[Dave Tapuska]

I had the same thoughts that Jeremy had when I first saw this. And I thought perhaps there was some intense discussion on it that I had missed. This definitely seems like we now are just going all in on layering violations. Chasing the highest benchmark you can get to isn't always the best course of action. Things like this use to be discussed prior on platform-dev. 

Dave

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CACuR13ejLKx5hqSHVuX8CDBdzUOsGBn1v-HwGg-j%2Bw0R0JVebg%40mail.gmail.com.

[Daniel Cheng]

I guess I should have said something earlier, but when I first saw the CLs, I assumed that this had been discussed somewhere :)

There's going to be a bunch of new getters and setters (and Members)
showing up, but they already existed through template magic; they are
just becoming visible now and given names and types (as opposed to being
string-keyed, which risks type confusion). We expect a tiny further
decrease in binary size and no significant change in compilation time.

This isn't quite correct; supplements were never string keyed. It took me some time to track this down and verify, since the CL that changed this wasn't associated with any bug. But we don't define any specific traits for `const char*` so we just hashed the pointer value directly (though arguably this is somewhat of a footgun too :).

While it's true that those getters and setters existed through template magic before, it still provided a useful abstraction to avoid embedding a bunch of arbitrary things into core classes. Now, we have 36 (!!) forward-declared members on LocalDOMWindow and another 28 on Navigator: that seems like a pretty big encapsulation failure. Are all 64 of these really performance critical?

I think there's a balance to be struck here, and defaulting everything to a member of LocalDOMWindow/Document/LocalFrame/Navigator doesn't really seem like a maintainable approach, even if it's good for benchmarks.

Daniel

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAHgVhZVfoweHqjL3bMDNvNrFOAYKBfmJTtARG2nEt9BRGG2OBQ%40mail.gmail.com.

[Steinar H. Gunderson]

On Thu, Nov 27, 2025 at 04:42:58PM -0800, Daniel Cheng wrote:
> I think there's a balance to be struck here, and defaulting everything to a
> member of LocalDOMWindow/Document/LocalFrame/Navigator doesn't really seem
> like a maintainable approach, even if it's good for benchmarks.

To be clear, I considered the change to increase maintainability and
readability; benchmarks was only a part of the equation. Size decrease was a
nice bonus.

These members were there all along. It's just that they are more visible now
instead of being hidden away by compiler-generated code; I consider that a
good thing. We haven't created more layer violations -- if A is not allowed
to hold B, it shouldn't be allowed to do so through a Supplementable-like
system either IMO (and Supplementable had big warnings on it that it was
prone to type confusion if used with inheritance).

To put it another way: If you came to the current Blink code base with no
knowledge of the past, would anyone say that we should take a lot of the
members on ExecutionContext and stick them into an untyped hash table?
(If so, should e.g. OriginTrialContext have been part of this table,
which it wasn't before?) A lot of stuff was stuck into ExecutionContext's
Supplementable without even being in different layers, and I'm not sure what
distinguished them from the other Members that were there before. And I've
honestly never seen this pattern recommended anywhere else before; it looked
odd to me all along, which is why I invested time in removing it.

But as others have pointed out, it's water under the bridge. I guess it _is_
possible to revert it still if you wish, but it would be very conflict-prone.

[Yoav Weiss (@Shopify)]

As an external contributor, I found Supplementables and Supplements to be an extremely useful way of avoiding touching large headers that trigger a rebuild of large parts of Blink.

But more importantly, I think that such major decisions should be taken after a public discussion (where different folks can present their pro/con arguments) and not presented as "water under the bridge". It'd be good to aim for that next time. 

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/64st4nwknc2eq5vb7seb6ri5d6qb4l4nsqqpo7yvjhqhwmkepk%40hoayrxvm5v5k.

[Daniel Cheng]

On Fri, 28 Nov 2025 at 00:23, Steinar H. Gunderson <se...@chromium.org> wrote:

On Thu, Nov 27, 2025 at 04:42:58PM -0800, Daniel Cheng wrote:
> I think there's a balance to be struck here, and defaulting everything to a
> member of LocalDOMWindow/Document/LocalFrame/Navigator doesn't really seem
> like a maintainable approach, even if it's good for benchmarks.

To be clear, I considered the change to increase maintainability and
readability; benchmarks was only a part of the equation. Size decrease was a
nice bonus.

Based on the responses here, it does not seem like there is general agreement that this increases maintainability and readability.

 

These members were there all along. It's just that they are more visible now
instead of being hidden away by compiler-generated code; I consider that a
good thing. We haven't created more layer violations -- if A is not allowed
to hold B, it shouldn't be allowed to do so through a Supplementable-like
system either IMO (and Supplementable had big warnings on it that it was
prone to type confusion if used with inheritance).

Though `Supplementable` and `base::SupportsUserData` have sharp edges and could use improvement, the underlying abstraction still has value.

 

To put it another way: If you came to the current Blink code base with no
knowledge of the past, would anyone say that we should take a lot of the
members on ExecutionContext and stick them into an untyped hash table?
(If so, should e.g. OriginTrialContext have been part of this table,
which it wasn't before?) A lot of stuff was stuck into ExecutionContext's
Supplementable without even being in different layers, and I'm not sure what
distinguished them from the other Members that were there before. And I've
honestly never seen this pattern recommended anywhere else before; it looked
odd to me all along, which is why I invested time in removing it.

It's not a common pattern to embed dozens of forward-declared pointer fields in a class. It happens to work in Blink because the types live on the Oilpan heap, but non-Oilpan types would need additional indirections for this to work. In addition, the supplement pattern is widely used in Chrome; it's not unique to Blink. It's used in //content (via `base::SupportsUserData` in `WebContents`, `RenderFrameHost`, and `RenderFrame`; //content also supports a similar primitive called `DocumentUserData`); in addition, there's also a variant in //ui that supports stronger typing at the cost of additional magic.

Code search claims 290+ non-test subclasses of `WebContentsUserData`; I do not think any //content/OWNER would approve a CL that added 290 opaque pointer fields, getters, and setters to `content::WebContentsImpl`.

But as others have pointed out, it's water under the bridge. I guess it _is_
possible to revert it still if you wish, but it would be very conflict-prone.

Unfortunately, it seems multiple people (myself included) assumed that this discussion had happened, and I apologize for that. But I think it was a mistake to remove the abstraction without more discussion, and that discussion should still happen.

Daniel