Closed
Bug 849263
Opened 12 years ago
Closed 12 years ago
MovingBoxes test spends lots of time updating overflow areas
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: perf)
Attachments
(2 files)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details |
https://raw.github.com/tumult/PerformanceBenchmarks/master/MovingBoxes.html (needs to be downloaded, can't be run from that URL) is very slow. This has regressed sometime since Firefox 4. I haven't figured out a regression range, but my guess is that it's either bug 524925 or bug 815666 since it's spending a massive amount of time inside OverflowChangedTracker::Flush (poor-man's profiling, i.e., breaking in gdb, showed only that and painting/compositing stuff). (I would expect bug 524925 to have put an O(N^2) performance problem in this testcase and bug 815666 to have fixed it, actually. Though I don't quite understand what bug 815666 is doing, and it doesn't actually seem quite right to me.)
Assignee | ||
Comment 1•12 years ago
|
||
Also, this came from:
http://blog.tumult.com/2013/02/28/transform-translate-vs-top-left/
Assignee | ||
Updated•12 years ago
|
Summary: MovingBoxes test is very slow → MovingBoxes test spends lots of time updating overflow areas
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #722835 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → dbaron
Assignee | ||
Comment 3•12 years ago
|
||
I still have some questions about the stuff OverflowChangedTracker::Flush is doing, but those can wait... at least until I eat breakfast, and perhaps longer.
Assignee | ||
Comment 4•12 years ago
|
||
"blocks" bug 815666 at least in that it fixes a mistake in bug 815666 that prevented that bug from making the performance optimization that it intended to. Probably regressed by bug 524925, though, but I haven't checked.
Blocks: 815666
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1)
> Also, this came from:
> http://blog.tumult.com/2013/02/28/transform-translate-vs-top-left/
The only contact info I've found on that would be replying on twitter to:
https://twitter.com/hypeapp/status/307272488138711041
which I'll probably try doing after this lands.
Assignee | ||
Comment 6•12 years ago
|
||
I'll attach the testcase for posterity in case that github repo changes or disappears.
Assignee | ||
Comment 7•12 years ago
|
||
Oh, and the actual regression range for the perf regression in the first phase of the test is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6338a8988917&tochange=7e4c2abb9fc9
i.e., bug 157681, which converted top/left changes to use the codepath from bug 524925. But the transform phase of the test probably regressed earlier.
(That said, there's probably also work to do to make this faster after this patch.)
Comment on attachment 722835 [details] [diff] [review]
Make OverflowChangedTracker actually sort by depth in the tree where it intended to.
Review of attachment 722835 [details] [diff] [review]:
-----------------------------------------------------------------
Ugh.
Attachment #722835 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Comment 10•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•