Closed
Bug 313437
Opened 19 years ago
Closed 7 years ago
Explicit local root model has bad human factors
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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
Reporter | ||
Comment 1•19 years ago
|
||
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•19 years ago
|
||
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.
Updated•19 years ago
|
Flags: testcase-
Updated•19 years ago
|
Whiteboard: [sg:want P2]
Comment 3•19 years ago
|
||
(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.
Reporter | ||
Comment 4•18 years ago
|
||
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•18 years ago
|
||
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
Comment 6•18 years ago
|
||
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 8•17 years ago
|
||
I am not working on the bug right now.
Assignee | ||
Updated•10 years ago
|
Assignee: general → nobody
Comment 9•7 years ago
|
||
Probably no longer valid now that everything is explicitly rooted, therefore closing as INCOMPLETE.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•