Bug 74423 - [MutationObservers] Avoid allocations if no observers are present
Summary: [MutationObservers] Avoid allocations if no observers are present
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-12-13 11:00 PST by Rafael Weinstein
Modified: 2019-05-02 16:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.86 KB, patch)
2011-12-13 11:07 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (13.27 KB, patch)
2011-12-13 14:59 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.