Add optional debug logging when we fall into/out of threaded scrolling

[Tim Horton]

It would be nice if we could know when we fall into/out of threaded scrolling.

I have a patch.

[Radar WebKit Bug Importer]

<rdar://problem/12089098>

[Tim Horton]

Created attachment 158100 [details]
patch

[Tim Horton]

I was calling it "fast" and "slow", but wasn't sure that was appropriate.

[Tim Horton]

Meh, disregard this. I really want to know *why* also, so there's more plumbing involved.

[Tim Horton]

Created attachment 158139 [details]
patch that prints reasons too

[Tim Horton]

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.

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Tim Horton]

Created attachment 158145 [details]
we needn't care about swapping to the main thread when we don't support threaded scrolling

[Simon Fraser (smfr)]

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!

[Tim Horton]

Created attachment 158186 [details]
simon's changes, hopefully

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Gyuyoung Kim]

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

[Early Warning System Bot]

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

[Build Bot]

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

[Build Bot]

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

[Tim Horton]

Created attachment 158371 [details]
build fix for non-threaded-scrolling platforms

[WebKit Review Bot]

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

[Build Bot]

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

[Tim Horton]

Created attachment 158378 [details]
ugh

[Tim Horton]

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.

[Simon Fraser (smfr)]

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?

[Tim Horton]

(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.

[Tim Horton]

(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!

[Tim Horton]

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

[Tim Horton]

(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.

[Tim Horton]

(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.

[Tim Horton]

(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)

[Tim Horton]

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.

[Tim Horton]

Created attachment 162605 [details]
smfr is changing things out from under me :)

[Tim Horton]

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.

[Tim Horton]

https://trac.webkit.org/changeset/127837

[James Robinson]

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.

[Tim Horton]

(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!

[James Robinson]

(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.

[James Robinson]

Reverted r127837 for reason:

Broke ScrollingCoordinator on chromium

Committed r128132: <http://trac.webkit.org/changeset/128132>

[Tim Horton]

Created attachment 163482 [details]
move enum to ScrollingCoordinator instead of ScrollingTreeState

[James Robinson]

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.

[Tim Horton]

(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?)

[James Robinson]

Yeah, I think that would help.

[James Robinson]

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.

[Tim Horton]

Created attachment 163738 [details]
don't rename all of the functions

[Simon Fraser (smfr)]

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.

[Tim Horton]

(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!

[Tim Horton]

http://trac.webkit.org/changeset/128521