Note: There are a few cases of duplicates in user autocompletion which are being worked on.

GC: investigate pause time increase between Aurora and Nightly

RESOLVED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: terrence, Assigned: billm)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12- unaffected, firefox13+ fixed, firefox14+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Split off from bug 731398.
Can you provide a bit more description? (And please nominate for tracking if you think it should be tracked.)
(Reporter)

Comment 2

6 years ago
The relevant text is: "From Aurora to Nightly the GC pause time seems to increase. The GC log shows only *RESET* GCs."

This is an incremental reset bug.  I opened this as a reminder to re-check the raytracer pause times after Bill fixes the other incremental reset bugs.
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox13: --- → +
tracking-firefox12: --- → +
Fixed for now by the disabling of IGC.
Blocks: 641025
status-firefox12: affected → unaffected
status-firefox13: affected → unaffected
tracking-firefox12: + → -
tracking-firefox13: + → -
(In reply to David Mandelin from comment #3)
> Fixed for now by the disabling of IGC.

I don't see that this is fixed. The pause time for the realtime raytracer on Aurora (3/13) is about 85msec and Nightly with IGC disabled about 120 msec.
(In reply to Gregor Wagner [:gwagner] from comment #4)
> (In reply to David Mandelin from comment #3)
> > Fixed for now by the disabling of IGC.
> 
> I don't see that this is fixed. The pause time for the realtime raytracer on
> Aurora (3/13) is about 85msec and Nightly with IGC disabled about 120 msec.

Which versions should we be tracking? I think Aurora was 12 when you originally reported this but it's 13 now.
(In reply to David Mandelin from comment #5)
> (In reply to Gregor Wagner [:gwagner] from comment #4)
> > (In reply to David Mandelin from comment #3)
> > > Fixed for now by the disabling of IGC.
> > 
> > I don't see that this is fixed. The pause time for the realtime raytracer on
> > Aurora (3/13) is about 85msec and Nightly with IGC disabled about 120 msec.
> 
> Which versions should we be tracking? I think Aurora was 12 when you
> originally reported this but it's 13 now.

Todays Aurora build is still 12. So 12 is not affected. Nightly is 14 but even with IGC disabled I see the regression. So I guess once Aurora 13 is out it will have the same regression.
I am on the update-channel for Aurora.
(In reply to Gregor Wagner [:gwagner] from comment #6)
> Todays Aurora build is still 12. So 12 is not affected. Nightly is 14 but
> even with IGC disabled I see the regression. So I guess once Aurora 13 is
> out it will have the same regression.

Thanks. OK, back on the radar.
status-firefox13: unaffected → affected
status-firefox14: --- → affected
tracking-firefox13: - → +
tracking-firefox14: --- → +
(Assignee)

Comment 9

5 years ago
Terrence tracked this down to the incremental GC landing. Before incremental GC landed, we were doing compartmental GCs. Now we're doing global GCs. I'll check on Monday if fixing that completely eliminates the regression.
Assignee: general → wmccloskey
Also the flight of the navigator seems to see this regression. The average GC time increased from 70msec to 120msec
(In reply to Bill McCloskey (:billm) from comment #9)
> Terrence tracked this down to the incremental GC landing. Before incremental
> GC landed, we were doing compartmental GCs. Now we're doing global GCs. I'll
> check on Monday if fixing that completely eliminates the regression.

Did it fix it?
(Assignee)

Comment 12

5 years ago
Created attachment 614607 [details] [diff] [review]
patch

There were two issues here. The first was that we were doing full GCs instead of compartment GCs. This was easy to fix.

The other problem is that the marking fast path got a little slower. This patch makes some changes that bring us back to pre-incremental GC speed on my machine. I had to remove one of the isOverBudget checks to do this. It's possible that this will cause some incremental GC slices to be too long. However, we can address that when incremental GC is ready to be turned on again.
Attachment #614607 - Flags: review?(igor)

Comment 13

5 years ago
Comment on attachment 614607 [details] [diff] [review]
patch

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

Is this a branch patch?

::: js/src/jsgcmark.cpp
@@ +1055,3 @@
>      }
>  
>      if (tag == ObjectTag) {

Use JS_LIKELY here so the compiler can optimize even without PGO. Without that GCC assumes that all values of a tag are likely. Based on that it may even push the code under "if" out of the fast path. With that change processMarkStackOther does not need JS_NEVER_INLINE.

@@ -1058,5 @@
>    scan_value_array:
>      JS_ASSERT(vp <= end);
>      while (vp != end) {
> -        budget.step();
> -        if (budget.isOverBudget()) {

I am surprised that this extra check costs so much. Have you tried to use JS_LIKELY in isOverBudget on the fast path and move budget.step() after the isOverBudget check?
(Assignee)

Comment 14

5 years ago
Created attachment 614906 [details] [diff] [review]
patch v2

The good news is that the JS_NEVER_INLINE turned out to be unnecessary. I added it along with some other stuff and never measured how much it mattered.

The bad news is that I added a JS_LIKELY to the (tag == ObjectTag) branch. That actually made us go a little bit slower (83ms instead of 81ms for a GC).

I also tried adding back the isOverBudget check to the scan_value_array section, but with JS_UNLIKELY around it. That increased the mark time to about about 100ms, which is about the same as it was without the JS_UNLIKELY.

I've seen JS_LIKELY help in some places, but unfortunately it doesn't seem to be having any effect here.

And regarding branches, I think we should try to land this on FF14 and aurora.
Attachment #614607 - Attachment is obsolete: true
Attachment #614906 - Flags: review?(igor)
Attachment #614607 - Flags: review?(igor)

Updated

5 years ago
Attachment #614906 - Flags: review?(igor) → review+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80635a1c62f
Target Milestone: --- → mozilla14
(Assignee)

Comment 16

5 years ago
Comment on attachment 614906 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): Bug 641025
User impact if declined: Slower GC mark times, longer GC pauses
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): Pretty low risk. It moves some code around and makes a few semantic changes that should only affect incremental GC, which is disabled for FF13.
String changes made by this patch: None
Attachment #614906 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c80635a1c62f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #614906 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[Triage comment]
low risk, incremental GC disabled in 13 so we shouldn't see issues there.
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/480c0fe7f591

Updated

5 years ago
status-firefox13: affected → fixed
status-firefox14: affected → fixed
Is this something QA can verify?
Whiteboard: [qa?]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)
> Is this something QA can verify?

Taking silence as a 'no' and marking [qa-]. Please mark as [qa+] if there is something QA can do to verify this fix.
Whiteboard: [qa?] → [qa-]
Depends on: 759880
You need to log in before you can comment on or make changes to this bug.