Closed Bug 1246641 Opened 4 years ago Closed 4 years ago

Huge increase of CPU and memory use in JS demo after bug 1221616

Categories

(Core :: Graphics, defect)

45 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- unaffected
firefox45 + fixed
firefox46 + fixed
firefox47 + fixed

People

(Reporter: epinal99-bugzilla2, Assigned: bas.schouten)

References

()

Details

(Keywords: perf, regression, Whiteboard: [mozfr-community],gfx-noted)

Attachments

(3 files)

Attached file memory-report.json.gz
STR: load this JS demo http://js1k.com/2015-hypetrain/demo/2198 and play with it.

Result: CPU use is high (25% one core) and memory use never stops.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b4c27ab3cafeef188e84a704cbe72f27419a1eea&tochange=e4944eb37eecb1587c5acec96a14bd7f7d6fde57

Bas Schouten — Bug 1221616: Use ID2D1CommandList instead of a bitmap for temporary D2D drawing. r=jrmuizel

Before the patch, the perf of the demo was bad, it's clearly smoother now, but the ressources used are terrible.
Blocks: 1221616
Flags: needinfo?(bas)
Keywords: perf, regression
Whiteboard: [mozfr-community]
Whiteboard: [mozfr-community] → [mozfr-community],gfx-noted
Attachment #8717944 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8717944 [details]
MozReview Request: Bug 1246641: Also execute an occasional EndDraw for CommandLists used by non-operator OVER drawing. r=jrmuizel

https://reviewboard.mozilla.org/r/34361/#review31227
https://hg.mozilla.org/mozilla-central/rev/14406bcd91aa
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Milan, do you agree we should take it?

Tracking as it is a recent regression.
Flags: needinfo?(milan)
Comment on attachment 8717944 [details]
MozReview Request: Bug 1246641: Also execute an occasional EndDraw for CommandLists used by non-operator OVER drawing. r=jrmuizel

Approval Request Comment
[Feature/regressing bug #]: 1221616 (landed in 45)
[User impact if declined]: Higher resource usage with long command lists.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: We could probably construct an example where this leads to a  performance degradation (e.g., make the command list new max+1), but it doesn't look like we've seen anything in the tests.
[String/UUID change made/needed]:
Flags: needinfo?(milan)
Attachment #8717944 - Flags: approval-mozilla-beta?
Attachment #8717944 - Flags: approval-mozilla-aurora?
Comment on attachment 8717944 [details]
MozReview Request: Bug 1246641: Also execute an occasional EndDraw for CommandLists used by non-operator OVER drawing. r=jrmuizel

Taking it. Will probably be in 45 beta 7
Flags: needinfo?(bas)
Attachment #8717944 - Flags: approval-mozilla-beta?
Attachment #8717944 - Flags: approval-mozilla-beta+
Attachment #8717944 - Flags: approval-mozilla-aurora?
Attachment #8717944 - Flags: approval-mozilla-aurora+
I'm hitting conflicts trying to uplift this to beta. Can we get a rebased patch?
Flags: needinfo?(bas)
(In reply to Wes Kocher (:KWierso) from comment #11)
> I'm hitting conflicts trying to uplift this to beta. Can we get a rebased
> patch?

in danger of missing beta cycle
Flags: needinfo?(sledru)
Milan, can you help here? thanks
Flags: needinfo?(sledru) → needinfo?(milan)
(In reply to Carsten Book [:Tomcat] from comment #12)
> (In reply to Wes Kocher (:KWierso) from comment #11)
> > I'm hitting conflicts trying to uplift this to beta. Can we get a rebased
> > patch?
> 
> in danger of missing beta cycle

So this patch is not rebasable to Beta. It depends on code that is only in Aurora and can't really be uplifted. I will create a patch specifically for beta but it should be obvious that the only testing that will get is on beta. We will have to determine whether the risk presented by that (it's not a super high risk patch fwiw) is justified by the severity and occurrence of the bug. It should be noted this bug will occur on any canvas that does continuous drawing while not using operator over. Those canvas applications aren't common, but exist, and will inevitably eventually run out of address space.
OK, let's ship with this regression.
Here's the thing though - 45 is an ESR, so this (arguably not very common) memory leak will be out there for a very long time.
Flags: needinfo?(milan)
This patch is created specifically for beta, considering that, please do a quick re-review if I didn't make any silly mistakes Jeff.
Flags: needinfo?(bas)
Attachment #8722651 - Flags: review?(jmuizelaar)
Comment on attachment 8722651 [details] [diff] [review]
Similar patch for beta: Occasionally call EndDraw when using a lot of command lists.

Review of attachment 8722651 [details] [diff] [review]:
-----------------------------------------------------------------

Sure.
Attachment #8722651 - Flags: review?(jmuizelaar) → review+
As per Milan's comment.
Flags: needinfo?(sledru)
Comment on attachment 8722651 [details] [diff] [review]
Similar patch for beta: Occasionally call EndDraw when using a lot of command lists.

[Triage Comment]
taking it, should be in 45 beta 10.
Flags: needinfo?(sledru)
Attachment #8722651 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.