Bug 74423 – [MutationObservers] Avoid allocations if no observers are present

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rafael Weinstein]

In order to minimize perf impact when there are no mutation observers, the mutation observers dispatch mechanism should avoid the following if it can be quickly determined that no possible observers are present:

-creating the mutation record
-creating the mutation record interest group, which
  -stack allocates a hashmap of observers (which may turn out to be empty)

[Rafael Weinstein]

Created attachment 119043 [details]
Patch

[Adam Klein]

Comment on attachment 119043 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

Not sure if this'll be a problem, but I think this is supposed to come before the detailed changelog description.

> Source/WebCore/dom/CharacterData.cpp:194
>      OwnPtr<MutationObserverInterestGroup> mutationRecipients = MutationObserverInterestGroup::createForCharacterDataMutation(this);

This (and all the other similar lines) can be collapsed into the if statement.

> Source/WebCore/dom/ChildListMutationScope.cpp:247
> +        m_accumulations.set(target, nullptr);

As pointed out by Darin Adler in a recent review, we could skip this step, or only run it if !NDEBUG, as it's only used by assertions.  Instead, we'd just use the non-presence of the Accumulator as a signal that we don't need to do anything.

> Source/WebCore/dom/Element.cpp:187
> +    if (mutationRecipients)

Collapse into if statement

[Ojan Vafai]

Comment on attachment 119043 [details]
Patch

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

Feel free to committ after addressing Adam's and my feedback

> Source/WebCore/dom/WebKitMutationObserver.cpp:167
> +    return createIfNeeded(target, WebKitMutationObserver::ChildList, nullAtom, 0 /* oldValueFlag */);

The typical webkit way of doing this is to create a local oldValueFlag variable instead of adding a comment.

[Rafael Weinstein]

Comment on attachment 119043 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        Reviewed by NOBODY (OOPS!).
> 
> Not sure if this'll be a problem, but I think this is supposed to come before the detailed changelog description.

done

>> Source/WebCore/dom/CharacterData.cpp:194
>>      OwnPtr<MutationObserverInterestGroup> mutationRecipients = MutationObserverInterestGroup::createForCharacterDataMutation(this);
> 
> This (and all the other similar lines) can be collapsed into the if statement.

done

>> Source/WebCore/dom/Element.cpp:187
>> +    if (mutationRecipients)
> 
> Collapse into if statement

done

>> Source/WebCore/dom/WebKitMutationObserver.cpp:167
>> +    return createIfNeeded(target, WebKitMutationObserver::ChildList, nullAtom, 0 /* oldValueFlag */);
> 
> The typical webkit way of doing this is to create a local oldValueFlag variable instead of adding a comment.

done

[Rafael Weinstein]

Created attachment 119088 [details]
Patch

[Adam Klein]

Comment on attachment 119088 [details]
Patch

Clearing flags on attachment: 119088

Committed r102721: <http://trac.webkit.org/changeset/102721>

[Adam Klein]

All reviewed patches have been landed.  Closing bug.