Closed Bug 488414 Opened 11 years ago Closed 11 years ago

TM: investigate if it is sound to call js_GenerateShape from the trace

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 5 obsolete files)

Currently record_SetPropHit adds to the trace code to call js_AddProperty. That in turn may call js_AddScopeProperty which in turns calls js_GenerateShape. When shape overflows, the latter calls js_GC().

Is it expected?
Looks like a js_LeaveTrace is needed, at the least.

/be
Flags: blocking1.9.1?
I suspect a good solution to this and other cache bugs would be to change js_GenerateShape not to call js_GC. Rather on shape overflow it should just disable the cache and schedule the GC to happen at a later point.
Along the same lines, we shouldn't renumber shapes at every GC. We should use a separate heuristics for that.
I am going to implement that idea of just scheduling, not running, the GC from js_GenerateShape. It still would mean that the trace that calls js_AddProperty must guard that shape has not overflow, but that is simple.
Assignee: general → igor
Blocks: 488731
Priority: -- → P2
Flags: blocking1.9.1? → blocking1.9.1+
If we want this for 3.5 it has to be P1 and blocking b4 IMO. This is going to allow GC-ing during trace execution. Lots of potential fallout. Brendan thinks it might be possible to hack this short-term though.
P1 for now
Priority: P2 → P1
To Robert Sayre:

I have a patch that I can finish today, but it depends on 487039. If that sounds OK, I would suggest to make that bug P1 as well.
Igor: if that's the case, please re-nominated bug 487039 with a reason in that bug.
Attached patch v1 - work in progress (obsolete) — Splinter Review
The patch implements that idea of just scheduling, not calling the GC on the shape overflow. 

The most unclear part is js_AddProperty built-in changes. Currently the function returns false both to indicate OOM error and simple property mismatch. The jited code guards towards the OOM_EXIT on false. Assuming that this is the right thing to do, the patch also make function to return false on shape overflow.
(In reply to comment #9)
> Created an attachment (id=373478) [details]
> v1 - work in progress

This patch is build on top of the patch from bug 487039 comment 19.
Attachment #373478 - Attachment is patch: true
Attachment #373478 - Attachment mime type: application/octet-stream → text/plain
Attached patch v2 - work in progress (obsolete) — Splinter Review
The new version fixes the nested GC lock that v1 has and makes sure that single-threaded build compiles and avoids useless gcLocked flag in function signatures.
Attachment #373478 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Asking for comments on js_AddProperty built-in changes - is just returning false on shape overflow is the right thing there?
Attachment #373479 - Attachment is obsolete: true
Attachment #373567 - Flags: review?(gal)
Attachment #373567 - Flags: review?(gal)
Attachment #373567 - Flags: review?(brendan)
Attachment #373567 - Flags: review+
Comment on attachment 373567 [details] [diff] [review]
v3

I like it. The "Updated by js_GenerateShape" is unclear to me. Could you extend that comment? Brendan should look at this too. Shouldn't we use ATOMIC_INC from PR for concurrent updates to shapeGen?
Attached patch v4 (obsolete) — Splinter Review
The new patch updates comments to make it clear that, compared with js_GenerateShape, js_RegenerateShapeForGC does not need to do atomic increments.

It also fixed a bug where gcIsNeeded would be cleared by SET_SLOT pseudo-GC runs.
Attachment #373567 - Attachment is obsolete: true
Attachment #373577 - Flags: review?(brendan)
Attachment #373567 - Flags: review?(brendan)
Comment on attachment 373577 [details] [diff] [review]
v4

>+#ifdef JS_THREADSAFE
>+# define js_TriggerAllOperationCallbacks(rt, gcLocked) \
>+    js_TriggerAllOperationCallbacks (rt, gcLocked)
>+#else
>+# define js_TriggerAllOperationCallbacks(rt, gcLocked) \
>+    js_TriggerAllOperationCallbacks (rt)
>+#endif

The space hack is a little too cute, but why use it at all #ifdef JS_THREADSAFE? Rather, define the macro only for !JS_THREADSAFE.

>+js_IsDisabledPropertyCache(JSContext *cx)

Rename to js_IsPropertyCacheDisabled.

>+#ifdef JS_THREADSAFE
>+# define js_GenerateShape(cx, gcLocked)    js_GenerateShape (cx, gcLocked)
>+#else
>+# define js_GenerateShape(cx, gcLocked)    js_GenerateShape (cx)
>+#endif

Same comment as above.

>@@ -3445,29 +3462,31 @@ js_GC(JSContext *cx, JSGCInvocationKind 
> 
>         /*
>          * We assume here that killing links to parent and prototype objects
>          * does not create garbage (such objects typically are long-lived and
>          * widely shared, e.g. global objects, Function.prototype, etc.). We
>          * collect garbage only if a racing thread attempted GC and is waiting
>          * for us to finish (gcLevel > 1) or if someone already poked us.
>          */
>-        if (rt->gcLevel == 1 && !rt->gcPoke)
>+        if (rt->gcLevel == 1 && !rt->gcPoke &&! rt->gcIsNeeded)

&&! => && !

>             goto done_running;
> 
>         rt->gcLevel = 0;
>         rt->gcPoke = JS_FALSE;
>         rt->gcRunning = JS_FALSE;
> #ifdef JS_THREADSAFE
>         rt->gcThread = NULL;
> #endif
>         gckind = GC_LOCK_HELD;
>         goto restart_at_beginning;
>     }
> 
>+    /* Clear gcIsNeeded now, when we are about to start a normal GC cycle. */
>+    rt->gcIsNeeded = JS_FALSE;
>     JS_UNLOCK_GC(rt);
> 
> #ifdef JS_TRACER
>     if (JS_ON_TRACE(cx))
>         goto out;
> #endif

Is the gcIsNeeded = false premature in light of the JS_TRACER code?

>@@ -124,18 +121,17 @@ js_FillPropertyCache(JSContext *cx, JSOb
>     jsuword vword;
>     ptrdiff_t pcoff;
>     jsuword khash;
>     JSAtom *atom;
>     JSPropCacheEntry *entry;
> 
>     JS_ASSERT(!cx->runtime->gcRunning);
>     cache = &JS_PROPERTY_CACHE(cx);
>-    pc = cx->fp->regs->pc;
>-    if (cache->disabled || (cx->fp->flags & JSFRAME_EVAL)) {
>+    if (js_IsDisabledPropertyCache(cx) || (cx->fp->flags & JSFRAME_EVAL)) {

Separate bug: turning off the cache from eval is no longer necessary, given the scripsToGC-based garbage collection of eval-compiled scripts, your fix to avoid caching non-global natives at the end of the scope chain (subsuming with statement disabling), and other fixes. Or so it seems to me! Could you file if you concur? Thanks.

>+                        if (js_IsDisabledPropertyCache(cx)) {
>+                            /*
>+                             * js_GenerateShape could not recover from shape's

s/shape's/rt->shapeGen's/

>+  sprop_changed:
>+  oom_exit:
>     JS_UNLOCK_SCOPE(cx, scope);
>     return JS_FALSE;

Looks good -- could use do-while(0) with break to avoid gotos, not sure it's any simpler.

js_AddProperty is BOOL_RETRY (please correct me if I'm wrong) so failing causes the triggered trace to exit back to the interpreter, to retry the add.

>+#ifdef JS_THREADSAFE
>+# define js_TriggerGC(cx, gcLocked)    js_TriggerGC (cx, gcLocked)
>+#else
>+# define js_TriggerGC(cx, gcLocked)    js_TriggerGC (cx)
>+#endif

Ditto early comment on space hack confinement to !JS_THREADSAFE.

Great to have the deferred trigger machinery -- I needed it during Firefox 3 propcache dev era, should have done it then for shape gen at least (sans operation callback changes for Fx3.5).

/be
Attachment #373577 - Flags: review?(brendan) → review+
Attached patch v5 (obsolete) — Splinter Review
addressing all the issues from the comment 15

In js_AddProperty built-in I replaced all those different goto labels by single exit_trace. The build it is bool_retry indeed. An alternative version using do-while(0) (or rather for(;;) { ... return true; }) was uglier IMO due to excessive indentation.
Attachment #373577 - Attachment is obsolete: true
Attachment #373590 - Flags: review+
Pushed to tracemonkey>
Attached patch v6Splinter Review
The new patch adds missing JS_TREADSAFE ifdef around JS_ASSERT(cx->requestDepth > 0) in js_TriggerGC.
Attachment #373590 - Attachment is obsolete: true
Attachment #373632 - Flags: review+
https://hg.mozilla.org/tracemonkey/rev/7523cef76351
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/7523cef76351
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
The type should be uint32 not jsuint.

/be
You need to log in before you can comment on or make changes to this bug.