David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 14 years ago

When taking out-of-line slow paths, JaegerMonkey has to sync both halves of a fat value because the GC will explode on torn values. For example, an object with a string or undefined tag will either crash the GC immediately or later (because the object is not seen as live).

If we could conservatively scan the JS stack, we could avoid these extra stores. TM wants this too, if we ever combine the interp and TM stacks.

It's hard to pin a perf win on this, but given past experiments I'm guessing maybe 1-2%. It shouldn't be hard to investigate.

Relevant GC+JM bugs to revert with this change:
 * bug 582882
 * bug 580694
 * part of bug 581256

	Luke Wagner [:luke]
	Comment 1 • 14 years ago

So what's cool about the JS stack is that we know exactly where the values can lie; we don't have to call ConservativeGCStackMarker::markWord on every word on the stack, rather, we just need to replace the MarkValueRange calls in StackSpace::mark to a conservative version.

On 64-bit, even the conservative scanner won't save us if the 64-bit pointer is torn in half.  However, and sstangl should confirm this, it would be weird for us to only be syncing 32-bit values on 64-bit.  (For one, it seems like it would be a perf hazard for the reasons mentioned in bug 582152 comment 2.)  If it is the case that we sync whole 64-bit words (and those words are proper values), do we even need conservative marking on 64-bit?

	Sean Stangl [:sstangl]
	Comment 2 • 14 years ago

(In reply to comment #1)
Luke's comment is accurate: it is actually cheaper to sync both type and payload than it is to sync only one, since syncing only one requires an 8-byte load.

	Luke Wagner [:luke]
	Comment 3 • 14 years ago

(In reply to comment #2)
Thanks.

But, for reference, David just pointed out a reason why, even on 64-bit, we need a conservative scan: since we don't initialize [fp->base(), fp->slots() + script->nslots) on function entry and the method-jit avoids stack copies, you can have trash values (potentially from the non-value guts of a JSStackFrame) in [fp->base(), cx->regs->sp).

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 4 • 14 years ago

Gregor, mind reviewing the GC part of this patch?

	Gregor Wagner [:gwagner]
	Comment 5 • 14 years ago

Comment on attachment 461396 [details] [diff] [review]
fix

r+ for the GC part.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 14 years ago

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/c387ce88e5b0

	Igor Bukanov
	Comment 7 • 14 years ago

(In reply to comment #6)
> http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/c387ce88e5b0

This is wrong - one cannot assume in TraceRuntime that tracer is GCMarker. Moreover, the stack must be scanned conservatively even if the tracer is not the GC tracer or leak detection tools would get wrong results. 

AFAICS the solution is just to move the marking of JS stack into ConservativeGCStackMarker::markRoots. This way the conservative marker can finish the job and later the GC can assert that things are marked by it and assert if necessary.

	Gregor Wagner [:gwagner]
	Comment 8 • 14 years ago

Uh yeah Igor is right. I didn't think of the API call and traceRuntime is used for cycle collection debugging and heap dumps with another JSTracer. Sorry!

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 9 • 14 years ago

We realized this later and just moved the ConservativeGCStackMarker into JSTracer.

	Igor Bukanov
	Comment 10 • 14 years ago

(In reply to comment #9)
> We realized this later and just moved the ConservativeGCStackMarker into
> JSTracer.

This is not right at all at the tracer must be lightweight and can be used to to navigate just a part of the object graph. This has nothing to do with the conservative scanner. The right solution would be just to extend ConservativeGCStackMarker::markRoots with the code to do JS stack scan.

This will be compatible with couple of patches that I plan to land soon that substantially improves assert coverage for the conservative scanner.