Bug 93898

Summary: Add optional debug logging when we fall into/out of threaded scrolling
Product: WebKit Reporter: Tim Horton <thorton>
Component: Layout and RenderingAssignee: 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 Flags
patch
none
patch that prints reasons too
webkit-ews: commit-queue-
we needn't care about swapping to the main thread when we don't support threaded scrolling
simon.fraser: review-
simon's changes, hopefully
webkit-ews: commit-queue-
build fix for non-threaded-scrolling platforms
webkit.review.bot: commit-queue-
ugh
none
update log state regardless of tile border pref (!), fix a typo
simon.fraser: review+
patch
none
smfr is changing things out from under me :)
none
move enum to ScrollingCoordinator instead of ScrollingTreeState
none
don't rename all of the functions simon.fraser: review+

Description Tim Horton 2012-08-13 14:01:32 PDT
It would be nice if we could know when we fall into/out of threaded scrolling.

I have a patch.
Comment 1 Radar WebKit Bug Importer 2012-08-13 14:02:08 PDT
<rdar://problem/12089098>
Comment 2 Tim Horton 2012-08-13 14:12:57 PDT
Created attachment 158100 [details]
patch
Comment 3 Tim Horton 2012-08-13 14:14:55 PDT
I was calling it "fast" and "slow", but wasn't sure that was appropriate.
Comment 4 Tim Horton 2012-08-13 14:21:11 PDT
Meh, disregard this. I really want to know *why* also, so there's more plumbing involved.
Comment 5 Tim Horton 2012-08-13 16:03:53 PDT
Created attachment 158139 [details]
patch that prints reasons too
Comment 6 Tim Horton 2012-08-13 16:24:45 PDT
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 7 Early Warning System Bot 2012-08-13 16:25:08 PDT
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 8 Gyuyoung Kim 2012-08-13 16:38:15 PDT
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
Comment 9 Tim Horton 2012-08-13 16:45:51 PDT
Created attachment 158145 [details]
we needn't care about swapping to the main thread when we don't support threaded scrolling
Comment 10 Simon Fraser (smfr) 2012-08-13 17:26:27 PDT
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!
Comment 11 Tim Horton 2012-08-13 19:00:35 PDT
Created attachment 158186 [details]
simon's changes, hopefully
Comment 12 Early Warning System Bot 2012-08-13 19:12:58 PDT
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 13 WebKit Review Bot 2012-08-13 19:17:08 PDT
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 14 Gyuyoung Kim 2012-08-13 19:26:52 PDT
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 15 Early Warning System Bot 2012-08-13 19:33:15 PDT
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 16 Build Bot 2012-08-13 21:00:32 PDT
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 17 Build Bot 2012-08-13 21:22:41 PDT
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
Comment 18 Tim Horton 2012-08-14 11:06:20 PDT
Created attachment 158371 [details]
build fix for non-threaded-scrolling platforms
Comment 19 WebKit Review Bot 2012-08-14 11:16:51 PDT
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 20 Build Bot 2012-08-14 11:22:51 PDT
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
Comment 21 Tim Horton 2012-08-14 11:23:43 PDT
Created attachment 158378 [details]
ugh
Comment 22 Tim Horton 2012-09-05 09:53:57 PDT
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.
Comment 23 mitz 2012-09-05 10:23:00 PDT
(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 24 Simon Fraser (smfr) 2012-09-05 11:46:13 PDT
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?
Comment 25 Tim Horton 2012-09-05 11:50:08 PDT
(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.
Comment 26 Tim Horton 2012-09-05 11:50:39 PDT
(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 27 Tim Horton 2012-09-05 13:49:58 PDT
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
Comment 28 Tim Horton 2012-09-05 23:20:10 PDT
(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.
Comment 29 Tim Horton 2012-09-05 23:21:50 PDT
(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.
Comment 30 Tim Horton 2012-09-05 23:22:12 PDT
(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)
Comment 31 Tim Horton 2012-09-06 15:16:24 PDT
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.
Comment 32 Tim Horton 2012-09-06 15:27:26 PDT
Created attachment 162605 [details]
smfr is changing things out from under me :)
Comment 33 Tim Horton 2012-09-07 00:33:07 PDT
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.
Comment 34 Tim Horton 2012-09-07 00:45:50 PDT
https://trac.webkit.org/changeset/127837
Comment 35 James Robinson 2012-09-10 16:56:33 PDT
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.
Comment 36 Tim Horton 2012-09-10 17:07:33 PDT
(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!
Comment 37 James Robinson 2012-09-10 17:10:13 PDT
(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.
Comment 38 James Robinson 2012-09-10 17:12:10 PDT
Reverted r127837 for reason:

Broke ScrollingCoordinator on chromium

Committed r128132: <http://trac.webkit.org/changeset/128132>
Comment 39 Tim Horton 2012-09-11 16:56:59 PDT
Created attachment 163482 [details]
move enum to ScrollingCoordinator instead of ScrollingTreeState
Comment 40 James Robinson 2012-09-11 18:32:55 PDT
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.
Comment 41 Tim Horton 2012-09-11 18:50:20 PDT
(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?)
Comment 42 James Robinson 2012-09-11 19:14:56 PDT
Yeah, I think that would help.
Comment 43 James Robinson 2012-09-12 12:32:23 PDT
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.
Comment 44 Tim Horton 2012-09-12 16:59:11 PDT
Created attachment 163738 [details]
don't rename all of the functions
Comment 45 Simon Fraser (smfr) 2012-09-13 13:46:40 PDT
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.
Comment 46 Tim Horton 2012-09-13 14:18:55 PDT
(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!
Comment 47 Tim Horton 2012-09-13 16:05:16 PDT
http://trac.webkit.org/changeset/128521