Closed
Bug 397641
Opened 17 years ago
Closed 17 years ago
AsyncScrollPortEvent posted even if overflow/underflow event won't be dispatched
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
(Keywords: perf, regression)
Attachments
(1 file)
3.17 KB,
patch
|
roc
:
review+
MatsPalmgren_bugz
:
review+
MatsPalmgren_bugz
:
superreview+
roc
:
approval1.9+
|
Details | Diff | Splinter Review |
Currently nsGfxScrollFrameInner::PostOverflowEvent creates new AsyncScrollPortEvent even if overflow/underflow event won't be dispatched in nsGfxScrollFrameInner::FireScrollPortEvent(). When handling AsyncScrollPortEvents, there is also a FlushPendingNotifications(Flush_Layout) in nsGfxScrollFrameInner::AsyncScrollPortEvent::Run, that is causing lots of flushing during tdhtml. (See also https://bugzilla.mozilla.org/show_bug.cgi?id=365477#c16) I think copying the "is-overflow/underflow-event-needed" check from FireScrollPortEvent() might make sense. Those checks used to be there until Bug 365477 moved them to be only in FireScrollPortEvent(). Also, to handle HTML and XUL similarly, better to PostOverflowEvent() right before returning from Layout()/Reflow(). Makes code easier to follow, and reflow must now have happened before calling PostOverflowEvent. Currently when running tdhtml (iterations = 5), ~5700 AsyncScrollPortEvents are created, with the patch that drops to ~20. I think there is only one page in tdhtml which has scrollbars (at least when running the test on this machine). With tp2 where pages are larger (so that overflow really happens quite often), the change isn't as big. w/o patch ~2000, w patch ~400. Mats, do you think there is something the patch might break? At least I couldn't find any problems using the testcases in scrolling related bugs. And feel free to r+sr ;)
Attachment #282397 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 1•17 years ago
|
||
(In reply to comment #0) > Currently when running tdhtml (iterations = 5), ~5700 AsyncScrollPortEvents > are created And pretty much all of those call flush. Total flush count during tdhtml is ~14600, so if the patch is any good, there would be significantly less flushing.
Assignee | ||
Comment 2•17 years ago
|
||
(In reply to comment #1) >Total flush count during tdhtml is ~14600, That is, calls to nsIPresShell::FlushPendingNotifications. PresShell::DoFlushPendingNotifications is called a lot more often.
Assignee | ||
Updated•17 years ago
|
Attachment #282397 -
Flags: review?(mats.palmgren) → review?(roc)
Comment 3•17 years ago
|
||
Comment on attachment 282397 [details] [diff] [review] possible patch Good catch. This looks good to me, r+sr=mats but it would be good if roc@ have time for a quick look as well.
Attachment #282397 -
Flags: superreview+
Attachment #282397 -
Flags: review+
Updated•17 years ago
|
Blocks: 365477
Keywords: perf,
regression
Looks good, but is there a way to share that checking code?
Assignee | ||
Comment 5•17 years ago
|
||
The duplicated code isn't long and nsGfxScrollFrameInner::FireScrollPortEvent uses some of the variables later in the method, so making a common function to check whether !vertChanged && !horizChanged wouldn't be that useful.
Attachment #282397 -
Flags: review?(roc) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #282397 -
Flags: approval1.9?
Attachment #282397 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Assignee: nobody → Olli.Pettay
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•