Bug 74423

Summary: [MutationObservers] Avoid allocations if no observers are present
Product: WebKit Reporter: Rafael Weinstein <rafaelw>
Component: DOMAssignee: Rafael Weinstein <rafaelw>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, akkaran046, macpherson, ojan, rniwa, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 68729    
Attachments:
Description Flags
Patch
none
Patch none

Description Rafael Weinstein 2011-12-13 11:00:10 PST
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)
Comment 1 Rafael Weinstein 2011-12-13 11:07:20 PST
Created attachment 119043 [details]
Patch
Comment 2 Adam Klein 2011-12-13 11:42:53 PST
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
Comment 3 Ojan Vafai 2011-12-13 11:46:55 PST
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.
Comment 4 Rafael Weinstein 2011-12-13 14:59:05 PST
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
Comment 5 Rafael Weinstein 2011-12-13 14:59:35 PST
Created attachment 119088 [details]
Patch
Comment 6 Adam Klein 2011-12-13 17:03:07 PST
Comment on attachment 119088 [details]
Patch

Clearing flags on attachment: 119088

Committed r102721: <http://trac.webkit.org/changeset/102721>
Comment 7 Adam Klein 2011-12-13 17:03:10 PST
All reviewed patches have been landed.  Closing bug.