Closed Bug 313437 Opened 16 years ago Closed 3 years ago

Explicit local root model has bad human factors

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE
mozilla1.9alpha1

People

(Reporter: brendan, Unassigned)

References

()

Details

(Keywords: sec-want, Whiteboard: [sg:want P2])

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
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
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.
Flags: testcase-
Whiteboard: [sg:want P2]
(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.
Igor, this bug has your name all over it for fixing (mine for blame ;-).  Can you relieve my buglist of it?  Thanks,

/be
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. 
Assignee: brendan → 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.
I am not working on this.
Assignee: igor → general
I am not working on the bug right now.
Assignee: general → nobody
Probably no longer valid now that everything is explicitly rooted, therefore closing as INCOMPLETE.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.