Closed Bug 1460385 Opened 2 years ago Closed 2 years ago
Don't trace propid in Trace
Cycle Collector Children
59 bytes, text/x-review-board-request
Jan noticed in bug 1460052 comment 4 that the profile in bug 1242861 comment 42 was spending most of its time tracing a propid which is a jsid... but the cycle collector never cares about jsids! They can only either be non-GC things, or strings or symbols. We should just delete this line.
We're early in the cycle, and this patch is extremely simple, so we may want to backport it to beta if it really does fix 10 second hangs on Google Inbox.
Comment on attachment 8974510 [details] Bug 1460385 - Don't trace propid in TraceCycleCollectorChildren. https://reviewboard.mozilla.org/r/242846/#review248726 Uh, yeah.
Attachment #8974510 - Flags: review?(sphink) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/b830e5f05a78 Don't trace propid in TraceCycleCollectorChildren. r=sfink
Sounds like this could possibly be a significant perf win? Should we consider backporting this given what a simple patch it is?
See comment 2.
Users on reddit are noticing and reporting this issue as they update to Fx60. We may want to keep this on the dot release radar.
evilpie just posted this profile from reddit in #jsapi: https://perf-html.io/public/8c71da2a0e685c8d4106216977006dff7b416fb7/calltree/?hiddenThreads=6-4-3-2&thread=8&threadOrder=0-2-3-4-5-6-8-1-7&v=3 This is tracing of ObjectGroup property ids here: https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/js/src/gc/Marking.cpp#1462 Called here: https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/js/src/gc/Tracer.cpp#279
we're getting reports on sumo as well: https://support.mozilla.org/en-US/questions/1217102 https://perfht.ml/2KdgP5V https://perfht.ml/2wAjWDo
(In reply to Ryan VanderMeulen [:RyanVM] from comment #8) > Users on reddit are noticing and reporting this issue as they update to > Fx60. We may want to keep this on the dot release radar. What are they noticing? Long pauses on Google Inbox? (In reply to Jan de Mooij [:jandem] from comment #9) > evilpie just posted this profile from reddit in #jsapi: Yeah, that was also showing up in the Google Inbox profile from the other bug. I thought it was something that was calling back into the same code I patched here, but looking at it again, you are right, there's a separate place that is tracing these jsids. (In reply to [:philipp] from comment #10) > we're getting reports on sumo as well: > https://support.mozilla.org/en-US/questions/1217102 I looked at the second profile posted there, and it is similar to evilpie's. In other words, it is happening on that |TraceEdge(trc, &prop->id, "group_property");| line, which will unfortunately not be fixed by this patch.
I filed bug 1460636 for the group property issue.
Comment on attachment 8974510 [details] Bug 1460385 - Don't trace propid in TraceCycleCollectorChildren. Approval Request Comment [Feature/Bug causing the regression]: Unknown, but something seems to have made this code much slower in 60. [User impact if declined]: Multisecond pauses on some web pages. [Is this code covered by automated tests?]: Yes, we run this code all the time. [Has the fix been verified in Nightly?]: No. I don't think we have good steps to reproduce the issue, but the profiler results are clear. [Needs manual test from QE? If yes, steps to reproduce]: We still need STR, and if we had them, it seems like there are other similar issues that might still be causing problems. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: It just deletes a line of code that we don't need. If somehow my reasoning is wrong, it could cause leaks. Normally, I'd wait for a few Nightlys to make sure there's no problems, but this might need to get landed on Release, so I figure it is good to get testing on Beta quickly. [String changes made/needed]: None
Attachment #8974510 - Flags: approval-mozilla-beta?
Comment on attachment 8974510 [details] Bug 1460385 - Don't trace propid in TraceCycleCollectorChildren. Approved for 61.0b4. Hopefully it'll help, even if there's more follow-up work needed still in the other bug.
Attachment #8974510 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This fixes an issue on Google Inbox that is similar to bug 1449033, but it isn't quite the same issue. This patch should be very low risk, but we shipped this perf fault at least as far back as 59, so it doesn't feel critical enough to land on release. It should be fixed in Firefox 61.
You need to log in before you can comment on or make changes to this bug.