Closed Bug 852588 Opened 10 years ago Closed 6 years ago

Santa's workshop delivers few presents per minute

Categories

(Core :: SVG, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: scoobidiver, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: perf, Whiteboard: [ietestdrive][jwatt:invalidation] [in-the-wild])

In Chrome 25 or IE 10, Santa's workshop delivers around 53-67 presents per minute while only 5 in Nightly/20130318.
Depends on: 827915
Component: Graphics → SVG
Whiteboard: [ietestdrive][parity-chrome][parity-ie10] → [ietestdrive]
Depends on: 869611
Depends on: 870192
Depends on: 871106
No longer depends on: 871106
Now it's 70 presents per minute and drop off back to 5 once you resize the window.
Depends on: 875175
Whiteboard: [ietestdrive] → [ietestdrive][jwatt:invalidation]
Comment 1 is still applicable.
Win7 with HWA.
I got 131 presents per minute, even after resizing the window. But the elves have didn't have a body! Even the one that appears explaining the benchmark.
Chrome 29 hit 159 presents per minute, but the elves didn't have body and the boxes were black sometimes.
IE 10 only hit 122 presentes per minute, but there were no glitches.
bug 871172 tracks the missing bodies.
Whiteboard: [ietestdrive][jwatt:invalidation] → [ietestdrive][jwatt:invalidation] [in-the-wild]
I've debugged this a little. The problem is that we forget the LayerActivity of a JS-animated element if the element briefly gets removed from the document.

Santa's Workshop has a background layer, animated layers for the elves, and a foreground layer with the sign at the top and the tree at the bottom right. The elves have their transforms updated frequently by JS, so we give each their own layer.

Now, every 250ms, the testcase attempts to add a new elf using spawnElf(), and that calls zSortElves(). zSortElves removes the DOM elements for all the elves from the document, reorders them, and adds them back into the document. This destroys and re-creates the nsIFrames for the elves, which means that they start out without a LayerActivity, which means that the next time we paint, the elves don't get their own layers, and the whole scene is painted into one layer. This also means that we keep merging the tree and the sign into the background, only to split them out again on the next frame.

All of this wouldn't be a problem if Santa's workshop used CSS animations instead of JavaScript-powered ones. Or it could set will-change: transform...

How can we fix this? Do we want to?
I suppose we could store LayerActivity on the content node instead of on the frame, so that all frames of one element would have the same activity. That's not where LayerActivity belongs, but I don't think there's a lot of demand for having different layer activities on different frames of the same element. (For this solution we'd need a free nsINode bit, so it would need to wait for bug 981257 / bug 996796.)
Another way to fix it would be to make frame destruction asynchronous and either just keep around the same frame if the nsIContent as added in the same place, or, if we had access to both the old and the new frame at the same time, just copy over the LayerActivity from the old frame to the new one.
roc, do you have any thoughts on comment 5?
Flags: needinfo?(roc)
I think this is worth fixing.

Storing layer activity on the element means we would have to know which frame was active. But I suppose we could say that only the primary frame, or the outermost frame, can be active, and that would work well enough.

Maybe we could do that, and keep using the NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY bit on frames, but store LayerActivityProperty on the content node? And, when constructing a frame for a node, if the node has a LayerActivityProperty then add the NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY bit to the frame if it's the outermost? That would avoid requiring a new content state bit. I guess LayerActivity would hold an content node reference in this case.
Flags: needinfo?(roc)
Depends on: 1009478
I've attached a patch for keeping layer activity alive across reframes of an element to bug 1009478.
It's not enough to prevent the tree from repainting after a call to zSortElves(), unfortunately, but it looks like better ThebesLayer recycling (bug 913443) fixes that. After that, the remaining unnecessary repaints are:
 - The elves themselves are repainted whenever they are reframed
 - The presents are repainted after they appear because they start out inactive
 - Sometimes some snow flakes are repainted, I haven't tracked down the cause for that yet.
Depends on: 913443
testcase is gone now.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.