Bug 216528 - MutationObserverRegistration should be ref counted
Summary: MutationObserverRegistration should be ref counted
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on: 216507 217923
Blocks:
  Show dependency treegraph
 
Reported: 2020-09-14 22:38 PDT by Ryosuke Niwa
Modified: 2021-04-05 14:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (24.60 KB, patch)
2020-09-14 23:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (24.47 KB, patch)
2020-09-16 18:07 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-09-14 22:38:59 PDT
MutationObserverRegistration should be ref-counted so that both
regular registry and transient registries can reference it using Ref.
Comment 1 Ryosuke Niwa 2020-09-14 23:06:26 PDT
Created attachment 408795 [details]
Patch
Comment 2 Ryosuke Niwa 2020-09-15 15:20:44 PDT
Ping reviewers.
Comment 3 Darin Adler 2020-09-16 14:01:54 PDT
Comment on attachment 408795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408795&action=review

I’m sort of surprised by how many RELEASE_ASSERT there are in this patch.

> Source/WebCore/dom/MutationObserver.cpp:127
> +        return std::pair<Ref<Node>, Ref<MutationObserverRegistration>> { node.releaseNonNull(), makeRef(*registration) };

Using two sets of braces would be nicer than having type write out almost the entire return type a second time:

    return { { node.releaseNonNull(), makeRef(*registration) } };

> Source/WebCore/dom/MutationObserver.cpp:136
> +    RELEASE_ASSERT(!m_registrations.contains(&registration));
>      m_registrations.add(&registration);

Since we are doing a RELEASE_ASSERT, please avoid double hashing by asserting based on the return value of add rather than a separate hash table lookup.

> Source/WebCore/dom/MutationObserver.cpp:142
> +    RELEASE_ASSERT(m_registrations.contains(&registration));
>      m_registrations.remove(&registration);

Since we are doing a RELEASE_ASSERT, please avoid double hashing by asserting based on the return value of remove rather than a separate hash table lookup.

> Source/WebCore/dom/NodeRareData.h:278
> +    static constexpr size_t MutationObserverRegistryInlineSize = 4;

I suggest defining a type with this size in it rather than writing out the type every time:

    using RegistryVector = Vector<Ref<MutationObserverRegistration>, 4>;
    using RegistrySet= HashSet<Ref<MutationObserverRegistration>>;

The code will be less repetitive and might even be easier to read.
Comment 4 Ryosuke Niwa 2020-09-16 17:55:43 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 408795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408795&action=review
> 
> I’m sort of surprised by how many RELEASE_ASSERT there are in this patch.

Actually, hasRareData() checks in Node.cpp don't need to be release asserts since we'd crash cleanly these days.

> > Source/WebCore/dom/MutationObserver.cpp:127
> > +        return std::pair<Ref<Node>, Ref<MutationObserverRegistration>> { node.releaseNonNull(), makeRef(*registration) };
> 
> Using two sets of braces would be nicer than having type write out almost
> the entire return type a second time:
> 
>     return { { node.releaseNonNull(), makeRef(*registration) } };

Good point. Fixed.

> > Source/WebCore/dom/MutationObserver.cpp:136
> > +    RELEASE_ASSERT(!m_registrations.contains(&registration));
> >      m_registrations.add(&registration);
> 
> Since we are doing a RELEASE_ASSERT, please avoid double hashing by
> asserting based on the return value of add rather than a separate hash table
> lookup.

Done. RELEASE_ASSERT'ed isNewEntry. These checks are release asserts because m_registrations continues to use a raw pointer to MutationObserverRegistration. I think we can avoid this if we invent a special weak ptr for single-observer pattern we often have in WebKit.

> > Source/WebCore/dom/MutationObserver.cpp:142
> > +    RELEASE_ASSERT(m_registrations.contains(&registration));
> >      m_registrations.remove(&registration);
> 
> Since we are doing a RELEASE_ASSERT, please avoid double hashing by
> asserting based on the return value of remove rather than a separate hash
> table lookup.

Fixed.

> > Source/WebCore/dom/NodeRareData.h:278
> > +    static constexpr size_t MutationObserverRegistryInlineSize = 4;
> 
> I suggest defining a type with this size in it rather than writing out the
> type every time:
> 
>     using RegistryVector = Vector<Ref<MutationObserverRegistration>, 4>;
>     using RegistrySet= HashSet<Ref<MutationObserverRegistration>>;
> 
> The code will be less repetitive and might even be easier to read.

Sure, I'd use MutationObserverRegistryVector though since "RegistryVector" sounds very generic.
Comment 5 Ryosuke Niwa 2020-09-16 18:07:16 PDT
Created attachment 408973 [details]
Patch for landing
Comment 6 EWS 2020-09-16 18:53:26 PDT
Committed r267175: <https://trac.webkit.org/changeset/267175>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408973 [details].
Comment 7 Radar WebKit Bug Importer 2020-09-16 18:54:23 PDT
<rdar://problem/69031310>
Comment 8 WebKit Commit Bot 2020-10-19 15:08:55 PDT
Re-opened since this is blocked by bug 217923
Comment 9 Ryosuke Niwa 2020-10-19 16:03:42 PDT
I think I'm gonna fix this some other way.
Comment 10 Ryosuke Niwa 2020-10-26 20:37:46 PDT
Turns out that the perf regression on nytimes.com was caused by a crash. Will re-land this.