Bug 216528

Created attachment 408795 [details]
Patch

Ping reviewers.

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.

(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.

Created attachment 408973 [details]
Patch for landing

Committed r267175: <https://trac.webkit.org/changeset/267175>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408973 [details].

<rdar://problem/69031310>

Re-opened since this is blocked by bug 217923

I think I'm gonna fix this some other way.

Turns out that the perf regression on nytimes.com was caused by a crash. Will re-land this.