392602 - ActionMonkey: implement a new heuristic for JS_MaybeGC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Switching to MMgc obsoletes the old heuristic.  In bug 391211, the latest patch has JS_MaybeGC doing a collection only if GC_ZEAL is turned on; we can do better than that.

Offhand Brendan suggested using the amount of memory (number of pages) committed, which should be quite easy to track if MMgc doesn't already.

Comment 1

[Tommy Reilly]

What is the function of JS_MaybeGC?   Is it a hook for opportunistic GC?   Because MMgc is incremental we already have a huerstic to start an incremental GC if we've consumed some portion of existing memory.   Perhaps JS_MaybeGC should just perform an incremental marking increment if we're in the incremental marking phase.

Comment 2

[Brendan Eich [:brendan]]

Hey Tommy,

JS_MaybeGC is dumb as dirt. It's a hook for opportunistic GC but SpiderMonkey has had no incremental GC. ActionMonkey is not ready for incremental AFAIK. So for now we probably want JS_MaybeGC to keep trying to figure out when to do a global GC if there's some evidence it will collect a bunch of garbage.

That heuristic MMgc uses to decide when to start an incremental GC sounds like it could be scaled to work.

/be

Comment 3

[Tommy Reilly]

Sounds fine, maybe it could be written in a forward thinking way so that it calls IncrementalMark if we're in incremental mode.

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> That heuristic MMgc uses to decide when to start an incremental GC sounds like
> it could be scaled to work.

Yep.  This code appears in GC::AllocBlock and looks about right:

	uint64 now = GetPerformanceCounter();
	if(!marking && !collecting &&
		heapSizeAtLastAlloc > collectThreshold &&
		now - lastSweepTicks > kMarkSweepBurstTicks && 
		heapSizeAtLastAlloc < heap->GetTotalHeapSize()) 
	{
		if(incremental)
			StartIncrementalMark();
		else
			Collect();
	}

A separate heuristic is used in GC::AllocBlockIncremental.  Maybe we can factor both into a single, new, public method.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: patch for actionmonkey, v1

This patch sits atop 4-5 other patches that haven't landed yet.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: patch for actionmonkey-tamarin, v1

This patch sits on top of the patch in bug 395962.

Comment 7

[Tommy Reilly]

Looks good to me.   

Comment 8

[Edwin Smith]

Comment on attachment 293687 [details] [diff] [review]
patch for actionmonkey-tamarin, v1

Certianly nothing obviously wrong with it and harmless until you call MaybeGC, so + for that.  

kind of wish there was a good one-stop-shop for what the heuristic values all are (thresholds,etc) but thats not what this bug is about.

Comment 9

[Jason Orendorff [:jorendorff]]

Pushed changeset c138b1ab5626 to tamarin-central.

Comment 10

[Jason Orendorff [:jorendorff]]

Pushed changeset f4b554fa4d29 to actionmonkey.