Open Bug 624867 Opened 9 years ago Updated 1 year ago

Pauses in Flight of the Navigator due to MaybeGC() being called on every 20th script execution

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

REOPENED
Tracking Status
blocking2.0 --- -

People

(Reporter: pcwalton, Assigned: pcwalton)

References

(Blocks 1 open bug)

Details

(Whiteboard: [softblocker][not-ready])

Attachments

(2 files)

MaybeGC() gets called on every 20th script execution, causing jank in the Flight of the Navigator demo. I've created a patch to remove this call.
Attached patch Proposed patch.Splinter Review
Proposed patch attached. Baking on try server.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Attachment #502945 - Flags: review?(jst)
Attachment #502945 - Flags: review?(jst) → review+
After this, do we only do full gc if there is cycle collection with gc?
(Note, it is possibly to run scripts without causing any cycle collections)

Is it possible to have cross compartment wrappers or something which need to be 
collected occasionally using something like MaybeGC()?
(In reply to comment #2)
> After this, do we only do full gc if there is cycle collection with gc?
> (Note, it is possibly to run scripts without causing any cycle collections)

MaybeGC() can call either, depending on some logic in the JSAPI: http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#1825

> Is it possible to have cross compartment wrappers or something which need to be 
> collected occasionally using something like MaybeGC()?

Yeah, although the JS engine will call it automatically if it needs to I believe (although it's slower).
No notable regressions on tp4. Going to send to tracemonkey.
Attachment #502945 - Flags: approval2.0?
blocking2.0: --- → betaN+
Whiteboard: fixed-in-tracemonkey
Attachment #502945 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/d1adb7b245bd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out in http://hg.mozilla.org/mozilla-central/rev/f28ee131df7a

Was causing SunSpider regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-tracemonkey
Version: unspecified → Trunk
So, could this cause the regression because these MaybeGC calls happen usually
when we aren't running javascript, but just right after that,
for example right after the function which was run because of some timeout.
Now, if we take explicit MaybeGC calls out, GC may run more often while
we're running the tests.

I wonder if we could change (mNumEvaluations > 20). That 20 has been there
forever. Also, could we perhaps explicitly do "compartment only gc" more often
and then call MaybeGC every now and then?

/me still has no idea how to explicitly call only compartment gc.
I can make an api for you.
Andreas, that would be great. Could you post a patch to this bug perhaps?
I could at least play with such API a bit to see if we can improve responsiveness.

I'm also trying to figure out how the MaybeGC in nsJSContext::ScriptEvaluated
works with MaybeGC call in nsJSContext::DOMOperationCallback.
If it was just documented somewhere when OperationCallback is actually called...
Depends on: 628110
Just trying out something;
sunspider seems to trigger compartment gc occasionally, but
for some reason FOTN doesn't (or if it does, it happens very rarely).
Posted to tryserver to see if this regresses sunspider.

humph, want to try FOTN with this?
Patrick, do you have any other ideas how to fix this - I mean, how to cause
less GC, or faster GC?
smaug, break on js_GC and walk up the stack to see what heuristic triggered that GC. That should tell us why it doesn't hit faster compartment GCs.
I ran Olli's try server build and compared to trunk with FOTN, and on trunk I see micro-pauses every so often, which I assume are gc.  With this build I got at least 6 world-stopping pauses that lasted a second or so, and really froze the main thread.

I also tried on our new demo, but it has pretty different run-time characteristics than FOTN.  In it I noticed a few large pauses I didn't recognize, too, but I'm less confident to say that they are the result of this patch.
(In reply to comment #14)
> I ran Olli's try server build and compared to trunk with FOTN, and on trunk I
> see micro-pauses every so often, which I assume are gc.  With this build I got
> at least 6 world-stopping pauses that lasted a second or so, and really froze
> the main thread.
Um, that sounds bad.

Andreas, Patrick, do you have any ideas how to improve gc scheduling
without regressing perf tests?
Whiteboard: [softblocker]
Any GC differences with Ubuntu? I'm seeing horrible GC pauses still with my own projects specifically unique to Ubuntu (10.10 x64).
(In reply to comment #16)
> Any GC differences with Ubuntu? I'm seeing horrible GC pauses still with my own
> projects specifically unique to Ubuntu (10.10 x64).

The code is the same across platforms. Are you generating a lot of garbage? There isn't much we can do at the moment if you're actually generating a lot of garbage.

Incidentally, this bug is largely resolved for me with Andreas' patch to revamp the GC heuristics.
I think my GC problem that only happens with Firefox 4 on Ubuntu 10.10 x64 is related to scaling the canvas. I think there's some serious GC problem with nearest-neighbor scaling via the Mozilla CSS extension -moz-crisp-edges unique to Firefox on Ubuntu.
Can you file a separate bug on that?  It's unlikely to be related to this.
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.
blocking2.0: betaN+ → .x+
(er updating flag to "-" as per previous comment!)
blocking2.0: .x+ → -
Comment on attachment 502945 [details] [diff] [review]
Proposed patch.

Was backed out, thus is no longer approved!
Attachment #502945 - Flags: approval2.0+ → approval2.0-
I wanted to land this bug on cedar, but it was not obvious which patch(es) should land.  Please submit a patch which is ready to land on cedar (or push there yourself directly).
Whiteboard: [softblocker] → [softblocker][not-ready]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.