|
Description
Chris Dumez
2014-11-02 09:36:59 PST
Created attachment 240813 [details]
WIP Patch
Created attachment 240814 [details]
WIP Patch
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) 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
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
Created attachment 240857 [details]
WIP Patch
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
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
Created attachment 241044 [details]
WIP Patch
Rebased now that the dependency landed.
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
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
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
Created attachment 241449 [details]
WIP Patch
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.
Created attachment 241467 [details]
WIP Patch
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.
Created attachment 241471 [details]
Patch
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 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 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? > 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 on attachment 241471 [details]
Patch
I am working on addressing Antti's feedback.
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.
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 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. 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
Created attachment 241618 [details]
Patch
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.
Antti, does this latest iteration address your feedback? 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 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. > 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 on attachment 241618 [details]
Patch
anyway, lets try it out
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? (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. (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. Created attachment 241721 [details]
Patch
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 on attachment 241721 [details] Patch Clearing flags on attachment: 241721 Committed r176212: <http://trac.webkit.org/changeset/176212> All reviewed patches have been landed. Closing bug. 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. 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. (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. |