313437 - Explicit local root model has bad human factors

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

People forget to use it.

The JS_EnterLocalRootScope/JS_LeaveLocalRootScope pair can help, and they have a C++ helper in jsapi.h on the mozilla1.9alpha trunk.  But we should improve their underpinnings and make local rooting automatic:

- We want to avoid malloc churn, so should preallocate the first local root stack chunk within the JSContext, fusing allocations.

- As Igor has pointed out, we need to protect return values as they flow back to callers, even though their local root scope (if newborn) has closed -- and they may not be newborn, but "oldborn" and without any other strong ref, so hanging by a thread.

For mozilla1.8 we have patched the callers to pass a local root or equivalent as the "rval" or "vp" out parameter, but for 1.9 and the future, we need something better.  Ideally, it would not entrain garbage beyond the scope of the caller, or even the statement in which the call was made.

- We should do something sensible with JS_ClearNewbornRoots once we eliminate the  cx->newborn array.  I propose making it null the top local root scope's slots.  Thoughts?

/be

Comment 1

[Brendan Eich [:brendan]]

By "make local roots automatic", I mean what Igor suggested in another bug, and what the JNI (based on Netscape's JRI) does: native function, getter, setter, etc. calls automatically enter a local root scope before the call, leaving it just after.  This too has to be efficient.  We should aim to avoid regressing Ts, Txul, Tp, Tdhtml and other benchmarks.  We may need to optimize.

/be

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Just as a note: doing this will most likely entail actually tracking down the gcFreeList bug where one of the entries becomes a small integer (and thus causing pain and unhappiness when we try to use it as an address), since it seems that more local root scopes expose it more often.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> Just as a note: doing this will most likely entail actually tracking down the
> gcFreeList bug where one of the entries becomes a small integer (and thus
> causing pain and unhappiness when we try to use it as an address), since it
> seems that more local root scopes expose it more often.

Such a bug is probably just a GC hazard:  object is used, inappropriately GCed while still reachable from something that wasn't rooted properly, and then the chunk of memory on the freelist is written to.  It could even be fixed already.

Comment 4

[Brendan Eich [:brendan]]

Igor, this bug has your name all over it for fixing (mine for blame ;-).  Can you relieve my buglist of it?  Thanks,

/be

Comment 5

[Igor Bukanov]

I think a conservative GC must be considered seriously. At the end it may even have a lesser impact on API then other rooting models. 

Comment 6

[Igor Bukanov]

I think the title of this bug has to state "Semi-explicit local root model has bad human factors". That, the current model in SpiderMonkey is not explicit as through its newborn array and js_EnterLocalRootScope API it hides the fact that values must be rooted. On the other hand it hides that badly as recent examples demonstrates.

A model that would always require to store GC-things in roots would avoid most of those bugs. In C++ such model can be realized with class trickery in almost transparent way, but even in C with right API, preprocessor macros and asserts it should possible to realize it. For example, if instead of jsval* the API would only accept JSRootedLocation* with macros to initialize that JSRootedLocation, then it would not be easy to pass unrooted location.

Comment 7

[Igor Bukanov]

I am not working on this.

Comment 8

[Igor Bukanov]

I am not working on the bug right now.

Comment 9

[André Bargull [:anba]]

Probably no longer valid now that everything is explicitly rooted, therefore closing as INCOMPLETE.