WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
93898
Add optional debug logging when we fall into/out of threaded scrolling
https://bugs.webkit.org/show_bug.cgi?id=93898
Summary
Add optional debug logging when we fall into/out of threaded scrolling
Tim Horton
Reported
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.
Attachments
patch
(3.21 KB, patch)
2012-08-13 14:12 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
patch that prints reasons too
(22.59 KB, patch)
2012-08-13 16:03 PDT
,
Tim Horton
webkit-ews
: commit-queue-
Details
Formatted Diff
Diff
we needn't care about swapping to the main thread when we don't support threaded scrolling
(24.15 KB, patch)
2012-08-13 16:45 PDT
,
Tim Horton
simon.fraser
: review-
Details
Formatted Diff
Diff
simon's changes, hopefully
(26.69 KB, patch)
2012-08-13 19:00 PDT
,
Tim Horton
webkit-ews
: commit-queue-
Details
Formatted Diff
Diff
build fix for non-threaded-scrolling platforms
(26.83 KB, patch)
2012-08-14 11:06 PDT
,
Tim Horton
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
ugh
(27.02 KB, patch)
2012-08-14 11:23 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
update log state regardless of tile border pref (!), fix a typo
(31.06 KB, patch)
2012-09-05 09:53 PDT
,
Tim Horton
simon.fraser
: review+
Details
Formatted Diff
Diff
patch
(31.23 KB, patch)
2012-09-06 15:16 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
smfr is changing things out from under me :)
(31.32 KB, patch)
2012-09-06 15:27 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
move enum to ScrollingCoordinator instead of ScrollingTreeState
(28.71 KB, patch)
2012-09-11 16:56 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
don't rename all of the functions
(23.08 KB, patch)
2012-09-12 16:59 PDT
,
Tim Horton
simon.fraser
: review+
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-08-13 14:02:08 PDT
<
rdar://problem/12089098
>
Tim Horton
Comment 2
2012-08-13 14:12:57 PDT
Created
attachment 158100
[details]
patch
Tim Horton
Comment 3
2012-08-13 14:14:55 PDT
I was calling it "fast" and "slow", but wasn't sure that was appropriate.
Tim Horton
Comment 4
2012-08-13 14:21:11 PDT
Meh, disregard this. I really want to know *why* also, so there's more plumbing involved.
Tim Horton
Comment 5
2012-08-13 16:03:53 PDT
Created
attachment 158139
[details]
patch that prints reasons too
Tim Horton
Comment 6
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.
Early Warning System Bot
Comment 7
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
Gyuyoung Kim
Comment 8
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
Tim Horton
Comment 9
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
Simon Fraser (smfr)
Comment 10
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!
Tim Horton
Comment 11
2012-08-13 19:00:35 PDT
Created
attachment 158186
[details]
simon's changes, hopefully
Early Warning System Bot
Comment 12
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
WebKit Review Bot
Comment 13
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
Gyuyoung Kim
Comment 14
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
Early Warning System Bot
Comment 15
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
Build Bot
Comment 16
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
Build Bot
Comment 17
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
Tim Horton
Comment 18
2012-08-14 11:06:20 PDT
Created
attachment 158371
[details]
build fix for non-threaded-scrolling platforms
WebKit Review Bot
Comment 19
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
Build Bot
Comment 20
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
Tim Horton
Comment 21
2012-08-14 11:23:43 PDT
Created
attachment 158378
[details]
ugh
Tim Horton
Comment 22
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.
mitz
Comment 23
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.
Simon Fraser (smfr)
Comment 24
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?
Tim Horton
Comment 25
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.
Tim Horton
Comment 26
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!
Tim Horton
Comment 27
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
Tim Horton
Comment 28
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.
Tim Horton
Comment 29
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.
Tim Horton
Comment 30
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)
Tim Horton
Comment 31
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.
Tim Horton
Comment 32
2012-09-06 15:27:26 PDT
Created
attachment 162605
[details]
smfr is changing things out from under me :)
Tim Horton
Comment 33
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.
Tim Horton
Comment 34
2012-09-07 00:45:50 PDT
https://trac.webkit.org/changeset/127837
James Robinson
Comment 35
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
.
Tim Horton
Comment 36
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!
James Robinson
Comment 37
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.
James Robinson
Comment 38
2012-09-10 17:12:10 PDT
Reverted
r127837
for reason: Broke ScrollingCoordinator on chromium Committed
r128132
: <
http://trac.webkit.org/changeset/128132
>
Tim Horton
Comment 39
2012-09-11 16:56:59 PDT
Created
attachment 163482
[details]
move enum to ScrollingCoordinator instead of ScrollingTreeState
James Robinson
Comment 40
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.
Tim Horton
Comment 41
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?)
James Robinson
Comment 42
2012-09-11 19:14:56 PDT
Yeah, I think that would help.
James Robinson
Comment 43
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.
Tim Horton
Comment 44
2012-09-12 16:59:11 PDT
Created
attachment 163738
[details]
don't rename all of the functions
Simon Fraser (smfr)
Comment 45
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.
Tim Horton
Comment 46
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!
Tim Horton
Comment 47
2012-09-13 16:05:16 PDT
http://trac.webkit.org/changeset/128521
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug