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

REOPENED
Assigned to

Status

()

Core
DOM
REOPENED
7 years ago
6 years ago

People

(Reporter: pcwalton, Assigned: pcwalton)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

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

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 502945 [details] [diff] [review]
Proposed patch.

Proposed patch attached. Baking on try server.
Assignee: nobody → pwalton
Status: NEW → ASSIGNED
Attachment #502945 - Flags: review?(jst)

Updated

7 years ago
Attachment #502945 - Flags: review?(jst) → review+

Comment 2

7 years ago
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()?
(Assignee)

Comment 3

7 years ago
(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).
(Assignee)

Comment 4

7 years ago
No notable regressions on tp4. Going to send to tracemonkey.
(Assignee)

Updated

7 years ago
Attachment #502945 - Flags: approval2.0?

Updated

7 years ago
blocking2.0: --- → betaN+
(Assignee)

Updated

7 years ago
Whiteboard: fixed-in-tracemonkey

Updated

7 years ago
Attachment #502945 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/d1adb7b245bd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

7 years ago
Backed out in http://hg.mozilla.org/mozilla-central/rev/f28ee131df7a

Was causing SunSpider regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

7 years ago
Whiteboard: fixed-in-tracemonkey
Version: unspecified → Trunk

Comment 8

7 years ago
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.

Comment 9

7 years ago
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...

Updated

7 years ago
Depends on: 628110
Created attachment 509760 [details] [diff] [review]
maybecompartmentgc

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?

Comment 13

7 years ago
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]

Comment 16

7 years ago
Any GC differences with Ubuntu? I'm seeing horrible GC pauses still with my own projects specifically unique to Ubuntu (10.10 x64).
(Assignee)

Comment 17

7 years ago
(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.

Comment 18

7 years ago
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-

Comment 24

7 years ago
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).

Updated

7 years ago
Whiteboard: [softblocker] → [softblocker][not-ready]
Blocks: 702495
You need to log in before you can comment on or make changes to this bug.