On Tue, 9 Dec 2025 at 04:00, Michael Lippautz <[email protected]> wrote:
> (Got pinged offline, sorry for the late reply.) > > I don't have a strong opinion on the final design here but let me say that > hiding references behind a hashmap for layering is really not ideal for > performance or memory. It's really a footgun that is hidden behind > accessors that all look the same. From a pure performance and memory > perspective the current state on HEAD is already an improvement for all our > users out there. I'd be very careful with putting developer compilation > time or layering and abstractions (that even hide performance > characteristics!) above actual user experience. > I agree it's not obvious to newer devs with less experience in Blink this requires a map lookup. But with the exception of a single Supplement–which was already changed to not be a Supplement before any of this work to get rid of Supplement, my understanding is none of it was visible in benchmarks. Without a quantifiable performance win, I don't think it makes a lot of sense to trade off better layering and abstractions. The Slack discussion seems to have settled on a similar outcome: it does not make sense to get rid of Supplement and add layering violations without a performance win. HashMap itself could use many improvements–the state of the art has improved quite a bit since HashMap was initially written. While none of these improvements will ever make it as cheap as a direct field lookup, there's probably a fair amount we're leaving on the floor. > Process-wise, I wonder why this is considered to be reverted at all and > not just improved going forward? > The Slack discussion has explicit support from multiple people for reverting to the status quo to address the layering violations introduced by the Supplement removal. No other alternative has any such momentum behind it. I think we should try to improve Supplement, but that should not block returning the codebase to a state with the correct layering. The CLs had the necessary approvals after all. Blink intentionally has very wide owners at the top-level, but part of that is ensuring reviews are routed to the right reviewers. In some cases, that's more obvious–for example, I generally re-direct more complex style reviews–while in other cases, it's less so–e.g. here. Sometimes unintended oversights happen, i.e. in this case many of the members of the former platform architecture team all were vaguely aware this was happening, but (incorrectly) assumed that the removal had already been discussed at length. Despite this being unintentional, I do not think these CLs had the "spirit" of necessary approvals even if Gerrit considered them approved per the OWNERS files. Daniel > > -Michael > > On Tue, Dec 2, 2025 at 6:17 PM Jeremy Roman <[email protected]> wrote: > >> The relevant Blink objects are garbage-collected (the Supplementable may >> not be the unique owner, and in any case ownership doesn't define the order >> in which objects are finalized once unreferenced), so we don't have a >> defined finalization order in most cases, regardless. >> >> On Mon, Dec 1, 2025 at 9:08 PM Erik Chen <[email protected]> wrote: >> >>> Food for thought if folks are going to design Supplementable V2. >>> >>> base::SupportsUserData has some problems: >>> * construction is usually lazy, which results in non-deterministic >>> construction/destruction ordering >>> * no enforcement mechanism for dependency acquisition. easy to create >>> dependency cycles. >>> >>> We've taken a different approach with //chrome/browser. See: >>> * chrome/browser/ui/browser_window/public/browser_window_interface.h >>> * ui/base/unowned_user_data/unowned_user_data_host.h >>> * chrome/browser/ui/browser_window/public/browser_window_features.h >>> * chrome/browser/ui/browser_window/internal/browser_window_features.cc >>> >>> The basic premise is that we have explicit construction/destruction >>> ordering with dependency acquisition in BrowserWindowFeatures. These >>> features can register themselves with >>> BrowserWindowInterface::GetUnownedUserDataHost. Clients of >>> BrowserWindowInterface can obtain a specific feature by including the >>> header for the feature and then fetching from >>> BrowserWindowInterface::GetUnownedUserDataHost. The migration isn't >>> finished but all the major pieces are there if folks want to poke around. >>> This feature was in turn inspired by UnownedUserDataHost >>> <https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/UnownedUserDataHost.java;l=26> >>> from >>> chrome on android. >>> >>> On Monday, December 1, 2025 at 8:58:27 AM UTC-8 Steve Kobes wrote: >>> >>>> On Fri, Nov 28, 2025 at 4:20 AM Daniel Cheng <[email protected]> >>>> wrote: >>>> >>>>> >>>>> >>>>> >>>>>> >>>>>> 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. >>>>> >>>> >>>> Perhaps this is a good opportunity to design a Supplementable V2 that >>>> polishes the rough edges? >>>> >>> -- >> 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 [email protected]. >> To view this discussion visit >> https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CACuR13e3hB_OGcddO62SYanbLrnMQT%2BA4%3DOKh%2By6pgQpbNJA5w%40mail.gmail.com >> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CACuR13e3hB_OGcddO62SYanbLrnMQT%2BA4%3DOKh%2By6pgQpbNJA5w%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> > -- 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 [email protected]. To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAF3XrKr3CXpviOXetJW9WGbip2yhGnN7gvJ8JDeDvRw8dtvo%3Dg%40mail.gmail.com.
