ActionMonkey: implement a new heuristic for JS_MaybeGC

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

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

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

Comment 4

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

Updated

12 years ago
Blocks: 393023
Depends on: 391211
(Assignee)

Updated

12 years ago
Assignee: general → swaroopch
(Assignee)

Comment 5

12 years ago
This patch sits atop 4-5 other patches that haven't landed yet.
Assignee: swaroopch → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 6

12 years ago
This patch sits on top of the patch in bug 395962.
Attachment #293687 - Flags: review?(treilly)
(Assignee)

Updated

12 years ago
Attachment #293685 - Flags: review?(igor)

Updated

12 years ago
Attachment #293685 - Flags: review?(igor) → review+
(Assignee)

Updated

11 years ago
Attachment #293687 - Flags: review?(treilly) → review?(edwsmith)

Comment 7

11 years ago
Looks good to me.   

Comment 8

11 years ago
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.
Attachment #293687 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 9

11 years ago
Pushed changeset c138b1ab5626 to tamarin-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

11 years ago
Pushed changeset f4b554fa4d29 to actionmonkey.

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.