Closed Bug 1460385 Opened 2 years ago Closed 2 years ago

Don't trace propid in TraceCycleCollectorChildren

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 + fixed
firefox62 + fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [qf])

Attachments

(1 file)

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.
Whiteboard: [qf]
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 amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b830e5f05a78
Don't trace propid in TraceCycleCollectorChildren. r=sfink
https://hg.mozilla.org/mozilla-central/rev/b830e5f05a78
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Sounds like this could possibly be a significant perf win? Should we consider backporting this given what a simple patch it is?
Flags: needinfo?(continuation)
See comment 2.
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
(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.
Flags: needinfo?(continuation)
See Also: → 1460636
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+
See Also: → 1447391
See Also: → 1449033
Blocks: 1449033
See Also: 1449033
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.
Blocks: 1462054
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.