Summary: | Add optional debug logging when we fall into/out of threaded scrolling | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Tim Horton <thorton> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | andersca, dglazkov, gustavo, jamesr, mitz, philn, sam, simon.fraser, tonikitoo, webkit-bug-importer, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Tim Horton
2012-08-13 14:01:32 PDT
Created attachment 158100 [details]
patch
I was calling it "fast" and "slow", but wasn't sure that was appropriate. Meh, disregard this. I really want to know *why* also, so there's more plumbing involved. Created attachment 158139 [details]
patch that prints reasons too
Comment on attachment 158139 [details] patch that prints reasons too View in context: https://bugs.webkit.org/attachment.cgi?id=158139&action=review > Source/WebCore/page/scrolling/ScrollingTreeState.cpp:93 > - if (m_shouldUpdateScrollLayerPositionOnMainThread == shouldUpdateScrollLayerPositionOnMainThread) > + if (m_reasonsForUpdatingScrollLayerPositionOnMainThread == reasonsForUpdatingScrollLayerPositionOnMainThread) This is a possibly undesirable behavior change. Comment on attachment 158139 [details] patch that prints reasons too Attachment 158139 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13480942 Comment on attachment 158139 [details] patch that prints reasons too Attachment 158139 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13485662 Created attachment 158145 [details]
we needn't care about swapping to the main thread when we don't support threaded scrolling
Comment on attachment 158145 [details] we needn't care about swapping to the main thread when we don't support threaded scrolling View in context: https://bugs.webkit.org/attachment.cgi?id=158145&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.h:154 > - void setShouldUpdateScrollLayerPositionOnMainThread(bool); > + void setReasonsForUpdatingScrollLayerPositionOnMainThread(unsigned); Setting "reasons for" doesn't necessarily imply that it will actually do anything, so I'd prefer staying closer to the old name. Also, using unsigned for a bitmask of known enum values is pretty icky. Typedef please! Created attachment 158186 [details]
simon's changes, hopefully
Comment on attachment 158186 [details] simon's changes, hopefully Attachment 158186 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13492350 Comment on attachment 158186 [details] simon's changes, hopefully Attachment 158186 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13489446 Comment on attachment 158186 [details] simon's changes, hopefully Attachment 158186 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13491392 Comment on attachment 158186 [details] simon's changes, hopefully Attachment 158186 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13489453 Comment on attachment 158186 [details] simon's changes, hopefully Attachment 158186 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13485720 Comment on attachment 158186 [details] simon's changes, hopefully Attachment 158186 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13503010 Created attachment 158371 [details]
build fix for non-threaded-scrolling platforms
Comment on attachment 158371 [details] build fix for non-threaded-scrolling platforms Attachment 158371 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13488721 Comment on attachment 158371 [details] build fix for non-threaded-scrolling platforms Attachment 158371 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13501303 Created attachment 158378 [details]
ugh
Created attachment 162272 [details]
update log state regardless of tile border pref (!), fix a typo
I'm still not sure we want to take this approach, but the log channel approach seems like a dead end. Ideally, we'd be able to hand structured data to clients, but I haven't had a chance to look into how to make that work yet, and it seems likely that that approach will involve equal or greater amounts of plumbing.
In either case, we will still need to keep the "reason" for dropping to main-thread scrolling around, which this patch does.
(In reply to comment #22) > Created an attachment (id=162272) [details] > I'm still not sure we want to take this approach, but the log channel approach seems like a dead end. Why? Even if you are not using a logging channel, you should use WTF logging, and definitely not write to stdout. Comment on attachment 162272 [details] update log state regardless of tile border pref (!), fix a typo View in context: https://bugs.webkit.org/attachment.cgi?id=162272&action=review > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:392 > + if (!supportsFixedPositionLayers() && frameView->hasFixedObjects()) > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasFixedObjectsWithoutSupportingFixedLayers; > + if (supportsFixedPositionLayers() && hasNonLayerFixedObjects(frameView)) > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasNonLayerFixedObjects; The cool kids are calling these "viewport constrained" objects now. > Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:353 > + reasonsDescription += "fixed objects,"; and sticky? (In reply to comment #23) > (In reply to comment #22) > > Created an attachment (id=162272) [details] [details] > > I'm still not sure we want to take this approach, but the log channel approach seems like a dead end. > > Why? People seemed opposed to release-mode-log-channels. > Even if you are not using a logging channel, you should use WTF logging, and definitely not write to stdout. Ah! WTFLogAlways()? Ok. Will do. (In reply to comment #24) > (From update of attachment 162272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162272&action=review > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:392 > > + if (!supportsFixedPositionLayers() && frameView->hasFixedObjects()) > > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasFixedObjectsWithoutSupportingFixedLayers; > > + if (supportsFixedPositionLayers() && hasNonLayerFixedObjects(frameView)) > > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasNonLayerFixedObjects; > > The cool kids are calling these "viewport constrained" objects now. > > > Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:353 > > + reasonsDescription += "fixed objects,"; > > and sticky? Ahh, lots of conflicts. My tree is a bit out of date. Thanks, Simon! Comment on attachment 162272 [details] update log state regardless of tile border pref (!), fix a typo View in context: https://bugs.webkit.org/attachment.cgi?id=162272&action=review > Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:346 > + String reasonsDescription; Dino points out that this should use StringBuilder (In reply to comment #24) > (From update of attachment 162272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162272&action=review > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:392 > > + if (!supportsFixedPositionLayers() && frameView->hasFixedObjects()) > > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasFixedObjectsWithoutSupportingFixedLayers; > > + if (supportsFixedPositionLayers() && hasNonLayerFixedObjects(frameView)) > > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasNonLayerFixedObjects; > > The cool kids are calling these "viewport constrained" objects now. I'll fix this in a followup, since it involves renaming stuff that I'm not currently touching, and this patch is already big enough. (In reply to comment #28) > (In reply to comment #24) > > (From update of attachment 162272 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=162272&action=review > > > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:392 > > > + if (!supportsFixedPositionLayers() && frameView->hasFixedObjects()) > > > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasFixedObjectsWithoutSupportingFixedLayers; > > > + if (supportsFixedPositionLayers() && hasNonLayerFixedObjects(frameView)) > > > + reasonsForUpdatingScrollLayerPositionOnMainThread |= ScrollingTreeState::HasNonLayerFixedObjects; > > > > The cool kids are calling these "viewport constrained" objects now. > > I'll fix this in a followup, since it involves renaming stuff that I'm not currently touching, and this patch is already big enough. Or... not, since I don't want to have to change the string that I print, and I don't want it to be inconsistent with the reason. (In reply to comment #29) > Or... not, since I don't want to have to change the string that I print, and I don't want it to be inconsistent with the reason. (by that, I mean, I'll do it in this patch) Created attachment 162601 [details]
patch
After talking to Simon, the rename hasn't propagated through to the rest of WebCore yet (a lot of things still use "fixed" instead of "viewport constrained"), so I'm going to use "viewport constrained" in my output (so that I don't have to change other code which might depend on this output), but not change any of the other naming.
I'll change the other two logs (the ones not being introduced by this patch) to WTFLogAlways instead of printf in another patch in a few minutes.
Created attachment 162605 [details]
smfr is changing things out from under me :)
Comment on attachment 162605 [details]
smfr is changing things out from under me :)
Simon made one additional comment (HasViewportConstrainedObjectsWithoutSupportingFixedLayers -> HasNonCompositedViewportConstrainedObjects); I didn't mean to post this for review, just wanted the EWS to run through the revised patch. Will land shortly.
I should have paid more attention to this patch, but you basically commented out the chromium implementation of ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread() which (obviously) completely breaks our implementation. We don't set ENABLE(THREADED_SCROLLING): http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/features.gypi. (In reply to comment #35) > I should have paid more attention to this patch, but you basically commented out the chromium implementation of ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread() which (obviously) completely breaks our implementation. We don't set ENABLE(THREADED_SCROLLING): http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/features.gypi. Oh no! I assumed that ports that use another thread for scrolling would be the ports that used setShouldUpdateScrollLayerPositionOnMainThread would be the ports with THREADED_SCROLLING on, but apparently I was wrong :/ Feel free to roll out, as I'm OoO this evening. Sorry for the breakage! (In reply to comment #36) > (In reply to comment #35) > > I should have paid more attention to this patch, but you basically commented out the chromium implementation of ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThread() which (obviously) completely breaks our implementation. We don't set ENABLE(THREADED_SCROLLING): http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/features.gypi. > > Oh no! I assumed that ports that use another thread for scrolling would be the ports that used setShouldUpdateScrollLayerPositionOnMainThread would be the ports with THREADED_SCROLLING on, but apparently I was wrong :/ > Afraid so. ENABLE(THREADED_SCROLLING) is mac-only. > Feel free to roll out, as I'm OoO this evening. Sorry for the breakage! I think I'll do that to restore previous behavior. To fix this, I'd suggest you move the ReasonsForHavingGiganticlyLongLines enum values into ScrollingCoordinator itself and make sure that the cross-platform ScrollingCoordinator code isn't guarded - stuff that is specific to ScrollingTree can stay guarded. Reverted r127837 for reason: Broke ScrollingCoordinator on chromium Committed r128132: <http://trac.webkit.org/changeset/128132> Created attachment 163482 [details]
move enum to ScrollingCoordinator instead of ScrollingTreeState
Comment on attachment 163482 [details] move enum to ScrollingCoordinator instead of ScrollingTreeState View in context: https://bugs.webkit.org/attachment.cgi?id=163482&action=review I believe this'll work fine for chromium. I'll patch this in and test to be sure tomorrow. I have some naming nits now that I'm looking more closely, see below. > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:383 > +void ScrollingCoordinator::updateShouldUpdateScrollLayerPositionOnMainThreadReason() This is a confusing name to me. This function is updating the property of whether we can scroll off the main thread. From the name, I'd expect that this is only updating some bookkeeping state, not something really important to the functioning of ScrollingCoordinator. Do we have to put the bookkeeeping bit in the name? > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:447 > +void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThreadReason(ReasonForUpdatingScrollLayerPositionOnMainThreadFlags reasons) I have the same sort of problem here. It sounds like from the function name we're just passing a bit of metadata around, but we're actually controlling behavior. (In reply to comment #40) > (From update of attachment 163482 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163482&action=review > > I believe this'll work fine for chromium. I'll patch this in and test to be sure tomorrow. I have some naming nits now that I'm looking more closely, see below. > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:383 > > +void ScrollingCoordinator::updateShouldUpdateScrollLayerPositionOnMainThreadReason() > > This is a confusing name to me. This function is updating the property of whether we can scroll off the main thread. From the name, I'd expect that this is only updating some bookkeeping state, not something really important to the functioning of ScrollingCoordinator. Do we have to put the bookkeeeping bit in the name? > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:447 > > +void ScrollingCoordinator::setShouldUpdateScrollLayerPositionOnMainThreadReason(ReasonForUpdatingScrollLayerPositionOnMainThreadFlags reasons) > > I have the same sort of problem here. It sounds like from the function name we're just passing a bit of metadata around, but we're actually controlling behavior. I would be OK with not having the "Reason" in the function name, sure. (is that what you're suggesting?) Yeah, I think that would help. This patch does appear to work correctly in chromium. My comment was mostly about the naming. I also think it's a bit dangerous to conflate a debugging concept - knowing the reason for main thread scrolling - with a very important behavior concept - forcing scrolls to happen on the main thread. Created attachment 163738 [details]
don't rename all of the functions
Comment on attachment 163738 [details] don't rename all of the functions View in context: https://bugs.webkit.org/attachment.cgi?id=163738&action=review r=me but please consider suggested renames. > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:398 > + if (m_page->mainFrame()->document()->isImageDocument()) > + reasonsForUpdatingScrollLayerPositionOnMainThread |= IsImageDocument; What about media documents, PDFs etc? Do we care? > Source/WebCore/page/scrolling/ScrollingCoordinator.h:47 > +typedef unsigned ReasonForUpdatingScrollLayerPositionOnMainThreadFlags; It's odd that this typedef is outside class scope, but the flags are inside. Such a long name! How about MainThreadScrollingReasons? > Source/WebCore/page/scrolling/ScrollingCoordinator.h:132 > + enum ReasonForUpdatingScrollLayerPositionOnMainThread { And MainThreadScrollingReasonFlags? > Source/WebCore/page/scrolling/ScrollingTreeState.cpp:94 > + if ((bool)m_shouldUpdateScrollLayerPositionOnMainThread == (bool)reasons) > return; I think C++ will do the type coercion for you. (In reply to comment #45) > (From update of attachment 163738 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163738&action=review > > r=me but please consider suggested renames. > > > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:398 > > + if (m_page->mainFrame()->document()->isImageDocument()) > > + reasonsForUpdatingScrollLayerPositionOnMainThread |= IsImageDocument; > > What about media documents, PDFs etc? Do we care? This decision was made due to specific performance concerns about drawing a single image into a tiled drawing area (actually due to a complaint I made to Anders a few months ago). > > Source/WebCore/page/scrolling/ScrollingCoordinator.h:47 > > +typedef unsigned ReasonForUpdatingScrollLayerPositionOnMainThreadFlags; > > It's odd that this typedef is outside class scope, but the flags are inside. Such a long name! How about MainThreadScrollingReasons? > > > Source/WebCore/page/scrolling/ScrollingCoordinator.h:132 > > + enum ReasonForUpdatingScrollLayerPositionOnMainThread { > > And MainThreadScrollingReasonFlags? Sounds good! > > Source/WebCore/page/scrolling/ScrollingTreeState.cpp:94 > > + if ((bool)m_shouldUpdateScrollLayerPositionOnMainThread == (bool)reasons) > > return; > > I think C++ will do the type coercion for you. It will! Thanks! |