Bug 138292 – Throttle timers that change the style of elements outside the viewport

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Dumez]

We should consider throttling timers that change the style of non-user observable Elements to avoid wasting CPU cycles updating something the user cannot see anyway.

[Chris Dumez]

Created attachment 240813 [details]
WIP Patch

[Chris Dumez]

Created attachment 240814 [details]
WIP Patch

[Chris Dumez]

Some initial results:

Without patch:
- http://huffingtonpost.com:
 * 14-15% at top
 * 14-15% with banner out of viewport

- http://espn.go.com:
 * 11% at top
 * 11% at bottom

With patch: 
- http://huffingtonpost.com:
 * 14-15% at top (same)
 * 1.5% with banner out of viewport (~14% less)

- http://espn.go.com:
 * 11% at top (same)
 * 2% at bottom (~9% less)

[Build Bot]

Created attachment 240818 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Created attachment 240820 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 240857 [details]
WIP Patch

[Build Bot]

Created attachment 240861 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Created attachment 240886 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 241044 [details]
WIP Patch

Rebased now that the dependency landed.

[Build Bot]

Created attachment 241050 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Created attachment 241073 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Created attachment 241075 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 241449 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 241449 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 241467 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 241467 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 241471 [details]
Patch

[WebKit Commit Bot]

Attachment 241471 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 241471 [details]
Patch

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

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:347
>      impl().setPropertyInternal(static_cast<CSSPropertyID>(propertyInfo.propertyID), propValue, important, ec);
>      setDOMException(exec, ec);
> +
> +    // Choke point for interaction with style of element; notify DOMTimer of the event.
> +    if (auto* element = impl().parentElement())
> +        DOMTimer::scriptDidChangeStyleOfElement(*element);

I think this would be better done inside setPropertyInternal. There is can check if something actually changed.

> Source/WebCore/page/DOMTimer.cpp:271
> +void DOMTimer::scriptDidChangeStyleOfElement(const StyledElement& styledElement)
> +{
> +    handleInteractionWithElement([&styledElement] () {
> +        // Changing the style of an element that is outside the viewport should not
> +        // be observable by the user.
> +        return styledElement.isInsideViewport();
> +    });
>  }

I don't think this is really sufficient. It is perfectly possible to change style of an element that is not inside view and affect content that is in view (via style inheritance or layout mechanisms). Also this is called before the style is resolved and the style change itself might move the element to the view.

The key would be detecting if the style change triggers a repaint (or layer movement) that is within the viewport. That means that throttling state can't be resolved until at least style has been resolved.

[Chris Dumez]

Comment on attachment 241471 [details]
Patch

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

>> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:347
>> +        DOMTimer::scriptDidChangeStyleOfElement(*element);
> 
> I think this would be better done inside setPropertyInternal. There is can check if something actually changed.

The issue is that setPropertyInternal() is also called from inside WebKit. Here, we know this is a call from JavaScript.
You want to not report timers that make changes to visible elements as long as they did not actually change anything? (i.e. the value set is the same as the existing one).? If so, we could also have setPropertyInternal() return a boolean?

[Antti Koivisto]

> The issue is that setPropertyInternal() is also called from inside WebKit.
> Here, we know this is a call from JavaScript.
> You want to not report timers that make changes to visible elements as long
> as they did not actually change anything? (i.e. the value set is the same as
> the existing one).? If so, we could also have setPropertyInternal() return a
> boolean?

You could also just add a parameter to indicate setPropertyInternal was called from JS wrapper.

[Chris Dumez]

Comment on attachment 241471 [details]
Patch

I am working on addressing Antti's feedback.

[Chris Dumez]

Created attachment 241557 [details]
WIP Patch

New version taking Antti's comments into consideration:
- Now update timers throttling state on scroll and layout to make sure there is no user-observable side effect
- Check if the style actually changed or not, and take that into account for throttling

I will test more and clean up tomorrow but feedback is welcome.

[WebKit Commit Bot]

Attachment 241557 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 241557 [details]
WIP Patch

Attachment 241557 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4956131231268864

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 241563 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 241618 [details]
Patch

[WebKit Commit Bot]

Attachment 241618 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Antti, does this latest iteration address your feedback?

[Antti Koivisto]

Comment on attachment 241618 [details]
Patch

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

> Source/WebCore/page/DOMTimer.cpp:448
> +        // Skip elements that were removed from the document.
> +        if (!element->inDocument())
> +            continue;

Should we also remove these elements from m_elementsCausingThrottling?

> Source/WebCore/page/DOMTimer.h:92
> +        // Hold a reference to the elements in case they get removed from the
> +        // Document after the timer is throttled.
> +        HashSet<RefPtr<StyledElement>> m_elementsCausingThrottling;

Adding a HashSet to every DOM timer seems a bit heavy weight. Also you never seem to look up anything from this hash. Could it just be a Vector?

[Chris Dumez]

Comment on attachment 241618 [details]
Patch

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

>> Source/WebCore/page/DOMTimer.cpp:448
>> +            continue;
> 
> Should we also remove these elements from m_elementsCausingThrottling?

I hesitated. I don't know if it is worth it because we clear m_elementsCausingThrottling every time the timer fires. As a result, in the worst case, we only extend the lifetime of the StyledElement until the next timer firing. If we want to be stricter, we would need to append to the Elements that are no longer in the Document to a Vector and then remove them from the HashSet *after* the loop.

>> Source/WebCore/page/DOMTimer.h:92
>> +        HashSet<RefPtr<StyledElement>> m_elementsCausingThrottling;
> 
> Adding a HashSet to every DOM timer seems a bit heavy weight. Also you never seem to look up anything from this hash. Could it just be a Vector?

The issue is that it is quite common for the script to change different CSS properties of the same Element. As a result, scriptDidUpdateStyleOfElement() is often called for the same StyledElement. Using a HashSet makes sure we don't have duplicate StyledElements. Having duplicates would be bad because we need to check if all these elements are still outside the viewport after each scroll / layout.

[Antti Koivisto]

> The issue is that it is quite common for the script to change different CSS
> properties of the same Element. As a result, scriptDidUpdateStyleOfElement()
> is often called for the same StyledElement. Using a HashSet makes sure we
> don't have duplicate StyledElements. Having duplicates would be bad because
> we need to check if all these elements are still outside the viewport after
> each scroll / layout.

As far as I can see you only ever replace m_elementsCausingThrottling completely:

m_elementsCausingThrottling = fireState.elementsChangedOutsideViewport();

Sure DOMTimerFireState should have a map internally but couldn't elementsChangedOutsideViewport() just return a vector?

[Antti Koivisto]

Comment on attachment 241618 [details]
Patch

anyway, lets try it out

[Antti Koivisto]

Comment on attachment 241618 [details]
Patch

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

> Source/WebCore/page/DOMTimer.cpp:289
> +    else {
> +        DOMTimerFireState::current->setScriptMadeNonUserObservableChanges();
> +        DOMTimerFireState::current->setChangedStyleOfElementOutsideViewport(styledElement);
> +    }

This is bit awkward. How about instead having

setScriptMadeNonUserObservableChangesToElementStyle(StyledElement& element)

to replace these two calls?

[Chris Dumez]

(In reply to comment #34)
> Comment on attachment 241618 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241618&action=review
> 
> > Source/WebCore/page/DOMTimer.cpp:289
> > +    else {
> > +        DOMTimerFireState::current->setScriptMadeNonUserObservableChanges();
> > +        DOMTimerFireState::current->setChangedStyleOfElementOutsideViewport(styledElement);
> > +    }
> 
> This is bit awkward. How about instead having
> 
> setScriptMadeNonUserObservableChangesToElementStyle(StyledElement& element)
> 
> to replace these two calls?

Sounds good to me, I will make this change.

[Chris Dumez]

(In reply to comment #32)
> > The issue is that it is quite common for the script to change different CSS
> > properties of the same Element. As a result, scriptDidUpdateStyleOfElement()
> > is often called for the same StyledElement. Using a HashSet makes sure we
> > don't have duplicate StyledElements. Having duplicates would be bad because
> > we need to check if all these elements are still outside the viewport after
> > each scroll / layout.
> 
> As far as I can see you only ever replace m_elementsCausingThrottling
> completely:
> 
> m_elementsCausingThrottling = fireState.elementsChangedOutsideViewport();
> 
> Sure DOMTimerFireState should have a map internally but couldn't
> elementsChangedOutsideViewport() just return a vector?

Oh, I see your point now, sorry. Yes, we only need to use a HashSet in fireState. We can then copy to a Vector in DOMTimer. I will make this change as well before landing.

[Chris Dumez]

Created attachment 241721 [details]
Patch

[WebKit Commit Bot]

Attachment 241721 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 241721 [details]
Patch

Clearing flags on attachment: 241721

Committed r176212: <http://trac.webkit.org/changeset/176212>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Layout testing coverage for this is coming in a follow-up patch. I need to add some test infrastructure to query if a timer is throttled or not.

[Antti Koivisto]

It might be good to add a setting for this feature and wire it to the debug menu. It would make it easier to test if any particular problem is caused by the throttling.

[Chris Dumez]

(In reply to comment #42)
> It might be good to add a setting for this feature and wire it to the debug
> menu. It would make it easier to test if any particular problem is caused by
> the throttling.

Agreed, I discussed this with Gavin already. I'll take care of this shortly.