Bug 138292 - Throttle timers that change the style of elements outside the viewport
Summary: Throttle timers that change the style of elements outside the viewport
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 138262 138339 138679 139140
Blocks: 138809 138838 138844 138873 138874 138875 138884 138914 138915
  Show dependency treegraph
 
Reported: 2014-11-02 09:36 PST by Chris Dumez
Modified: 2014-12-01 11:32 PST (History)
11 users (show)

See Also:


Attachments
WIP Patch (6.74 KB, patch)
2014-11-02 09:37 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (6.78 KB, patch)
2014-11-02 09:47 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (485.73 KB, application/zip)
2014-11-02 11:04 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (503.69 KB, application/zip)
2014-11-02 11:59 PST, Build Bot
no flags Details
WIP Patch (6.78 KB, patch)
2014-11-03 09:12 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (499.20 KB, application/zip)
2014-11-03 10:46 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (505.37 KB, application/zip)
2014-11-03 17:26 PST, Build Bot
no flags Details
WIP Patch (6.75 KB, patch)
2014-11-05 11:08 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (607.82 KB, application/zip)
2014-11-05 12:48 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (623.24 KB, application/zip)
2014-11-05 17:17 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (675.12 KB, application/zip)
2014-11-05 17:38 PST, Build Bot
no flags Details
WIP Patch (11.67 KB, patch)
2014-11-12 15:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (13.06 KB, patch)
2014-11-12 21:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (16.60 KB, patch)
2014-11-12 22:55 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (28.08 KB, patch)
2014-11-13 22:32 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (605.43 KB, application/zip)
2014-11-14 00:31 PST, Build Bot
no flags Details
Patch (29.35 KB, patch)
2014-11-14 13:26 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (29.50 KB, patch)
2014-11-17 10:09 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-11-02 09:36:59 PST
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.
Comment 1 Chris Dumez 2014-11-02 09:37:48 PST
Created attachment 240813 [details]
WIP Patch
Comment 2 Chris Dumez 2014-11-02 09:47:26 PST
Created attachment 240814 [details]
WIP Patch
Comment 3 Chris Dumez 2014-11-02 10:02:04 PST
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)
Comment 4 Build Bot 2014-11-02 11:04:51 PST
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
Comment 5 Build Bot 2014-11-02 11:59:57 PST
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
Comment 6 Chris Dumez 2014-11-03 09:12:02 PST
Created attachment 240857 [details]
WIP Patch
Comment 7 Build Bot 2014-11-03 10:46:18 PST
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
Comment 8 Build Bot 2014-11-03 17:26:48 PST
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
Comment 9 Chris Dumez 2014-11-05 11:08:28 PST
Created attachment 241044 [details]
WIP Patch

Rebased now that the dependency landed.
Comment 10 Build Bot 2014-11-05 12:48:16 PST
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
Comment 11 Build Bot 2014-11-05 17:17:55 PST
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
Comment 12 Build Bot 2014-11-05 17:38:57 PST
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
Comment 13 Chris Dumez 2014-11-12 15:44:07 PST
Created attachment 241449 [details]
WIP Patch
Comment 14 WebKit Commit Bot 2014-11-12 15:46:06 PST
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.
Comment 15 Chris Dumez 2014-11-12 21:38:46 PST
Created attachment 241467 [details]
WIP Patch
Comment 16 WebKit Commit Bot 2014-11-12 21:41:00 PST
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.
Comment 17 Chris Dumez 2014-11-12 22:55:34 PST
Created attachment 241471 [details]
Patch
Comment 18 WebKit Commit Bot 2014-11-12 22:58:10 PST
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.
Comment 19 Antti Koivisto 2014-11-13 10:41:59 PST
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.
Comment 20 Chris Dumez 2014-11-13 10:47:08 PST
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?
Comment 21 Antti Koivisto 2014-11-13 10:59:20 PST
> 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.
Comment 22 Chris Dumez 2014-11-13 12:15:26 PST
Comment on attachment 241471 [details]
Patch

I am working on addressing Antti's feedback.
Comment 23 Chris Dumez 2014-11-13 22:32:12 PST
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.
Comment 24 WebKit Commit Bot 2014-11-13 22:33:52 PST
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.
Comment 25 Build Bot 2014-11-14 00:31:51 PST
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.
Comment 26 Build Bot 2014-11-14 00:31:57 PST
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
Comment 27 Chris Dumez 2014-11-14 13:26:07 PST
Created attachment 241618 [details]
Patch
Comment 28 WebKit Commit Bot 2014-11-14 13:28:00 PST
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.
Comment 29 Chris Dumez 2014-11-14 14:43:11 PST
Antti, does this latest iteration address your feedback?
Comment 30 Antti Koivisto 2014-11-14 14:50:27 PST
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?
Comment 31 Chris Dumez 2014-11-14 15:05:06 PST
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.
Comment 32 Antti Koivisto 2014-11-17 04:50:11 PST
> 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?
Comment 33 Antti Koivisto 2014-11-17 04:50:35 PST
Comment on attachment 241618 [details]
Patch

anyway, lets try it out
Comment 34 Antti Koivisto 2014-11-17 04:57:19 PST
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?
Comment 35 Chris Dumez 2014-11-17 09:12:43 PST
(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.
Comment 36 Chris Dumez 2014-11-17 09:15:33 PST
(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.
Comment 37 Chris Dumez 2014-11-17 10:09:12 PST
Created attachment 241721 [details]
Patch
Comment 38 WebKit Commit Bot 2014-11-17 10:12:08 PST
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.
Comment 39 WebKit Commit Bot 2014-11-17 11:23:37 PST
Comment on attachment 241721 [details]
Patch

Clearing flags on attachment: 241721

Committed r176212: <http://trac.webkit.org/changeset/176212>
Comment 40 WebKit Commit Bot 2014-11-17 11:23:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 41 Chris Dumez 2014-11-17 14:56:52 PST
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.
Comment 42 Antti Koivisto 2014-11-18 13:28:39 PST
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.
Comment 43 Chris Dumez 2014-11-18 13:29:56 PST
(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.