Closed Bug 416601 Opened 17 years ago Closed 17 years ago

property cache can be left disabled after exit from a generator or trap handler

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(2 files, 8 obsolete files)

Currently the code that re-enables the property cache in the interpreter after an exit from middle of an with block does that only when en exception or error occur. But this ignores early exits forced by a trap handler or triggered by closing a generator.
Here is a test case demonstrating the problem with a debug build:

~/m/ff/mozilla $ cat ~/m/y.js
function f()
{
    with (Math) {
        yield 1;
    }
}

var iter = f();
iter.next();
iter.close();
~/m/ff/mozilla $ ~/m/ff/mozilla/js/src/Linux_All_DBG.OBJ/js ~/m/y.js
Assertion failure: JS_PROPERTY_CACHE(cx).disabled == fp->pcDisabledSave, at jsinterp.c:6741
trace/breakpoint trap
Attached patch v1 (obsolete) — Splinter Review
Backup: the main idea here is to add try notes for with, let and xml filter blocks so a generic stack unwind code will properly restore the stack without PutBlockObjects etc.
Comment on attachment 302361 [details] [diff] [review]
v1

Wondering whether there a more minimal fix for the short run? Also would like to avoid forcedReturn altogether (no additional overhead for a rare case). Could use a frame flag bit, but I wonder if you can't get rid of this by winding the logic tighter -- if it's set only by JSTRAP_RETURN cases, and tested only when setting catchable, perhaps the JSTRAP_RETURN cases could goto somewhere different.

/be
Attached patch v2 (obsolete) — Splinter Review
Here is another backup of a more minimal patch that do addrese compared with v1 the property cache enable/disable after an exit from a generator.

For that the patch adds JSOP_AFTERYIELD that together with JSOP_YIELD do the necessary number of enable/disable calls.

The patch also changes EmitJumpFixupPrefix to emit the fixups even for JSOP_RETURN to simplify the logic in the interpreter and avoid extra checks on return when there is no pending blocks.

This is untested.
Attachment #302361 - Attachment is obsolete: true
Depends on: 309894
Flags: blocking1.9+
Priority: -- → P1
Attached patch v3 (obsolete) — Splinter Review
Here is a smaller version that fixes the issue and does not move the code. The fix still alters the emitter to insert missing [leavewith]/[leaveblock] bytecodes before the return for simpler logic in the interpreter.
Attachment #302427 - Attachment is obsolete: true
Attachment #303032 - Flags: review?(brendan)
Blocks: 416439
Attached patch v4 (obsolete) — Splinter Review
Compared with v3 I renamed out2: to exit: given the other 2 labels, forced_return: and do_return: that the patch adds. Plus the patch turns (Alloc|Free)RawStack into static functions as they are not used outside jsinterp.c.

I will ask for a review after some testing.
Attachment #303032 - Attachment is obsolete: true
Attachment #303032 - Flags: review?(brendan)
Attachment #303048 - Flags: review?(brendan)
Comment on attachment 303048 [details] [diff] [review]
v4

In the patch I missed the fact that that AllocRawStack must set the mark after the potential time stamp header.
Attachment #303048 - Attachment is obsolete: true
Attachment #303048 - Flags: review?(brendan)
Attached patch v5 (obsolete) — Splinter Review
The new version removes that overzealous optimization of AllocRawStack the previous patch attempted, avoids that do_return label and do inlineCallCount != 0 checks only once on all code paths.
Attachment #303178 - Flags: review?(brendan)
Comment on attachment 303178 [details] [diff] [review]
v5

>+         * EmitNonLocalJumpFixup may add fixup bytecode to close an open try
>+         * blocks having finally clauses or to exit let blocks. We can't

Lose "an" before "open try blocks" and rewrap.

>+static JSClass *
>+IsEnteredWithOrBlock(JSContext *cx, JSObject *obj, int stackDepth)

Suggest IsActiveWithOrBlock, Entered is a bit awkward.

>+/*
>+ * Unwind the scope chain to match the given depth. The function sets fp->sp
>+ * on return to stackDepth.
>+ */
>+static JSBool
>+UnwindScope(JSContext *cx, JSStackFrame *fp, jsint stackDepth,

Maybe UnwindScopeChain is the better name. Not a big deal but thought I would mention it. Actually, since it unwinds fp->blockChain too, keep the name but change the comment to read "Unwind block and scope chains ...".

>+    for (;;) {
>+        obj = fp->scopeChain;
>+
>+        clasp = IsEnteredWithOrBlock(cx, obj, stackDepth);
>+        if (!clasp)
>+            break;

Nit: blank line after obj = fp->scopeChain; seems unnecessary, but blank line after break; is nice. Or no blank lines, it's not too dense here.

>+        for (i = CountWithBlocks(cx, fp); i != 0; --i)
>+            js_DisablePropertyCache(cx);

Really just want JS_PROPERTY_CACHE(cx).disabled += CountWithBlocks(cx, fp), with assertion against overflow. Could macroize to abstract a bit, but why bother? We're in jsinterp.c, it's all in the family.

>+        /*
>+         * To support generator_throw and to catch ignored exceptions,
>+         * fail if cx->throwing is set.
>+         */
>+        if (cx->throwing) {

Great to have this in the generator only case, now that filtering is done without leaving the interpreter.

>+            for (i = CountWithBlocks(cx, fp); i != 0; --i)
>+                js_EnablePropertyCache(cx);
>+            goto exit;

Same drill here, disabled -= CountWithBlocks().

>-         * Call debugger throw hook if set (XXX thread safety?).
>-         */
>+        /* Call debugger throw hook if set (XXX thread safety?). */

Since you are touching this comment, lose the ancient XXX.

>@@ -355,20 +355,20 @@ js_GetPrimitiveThis(JSContext *cx, jsval
>  */
> extern JSBool
> js_ComputeThis(JSContext *cx, jsval *argv);
> 
> /*
>  * NB: js_Invoke requires that cx is currently running JS (i.e., that cx->fp
>  * is non-null), and that vp points to the callee, |this| parameter, and
>  * actual arguments of the call. [vp .. vp + 2 + argc) must belong to the last
>- * JS stack segment that js_AllocStack or js_AllocRawStack allocated. The
>- * function may use the space available after vp + 2 + argc in the stack
>- * segment for temporaries so the caller should not use that space for values
>- * that must be preserved across the call.
>+ * JS stack segment that js_AllocStack allocated. The function may use the
>+ * space available after vp + 2 + argc in the stack segment for temporaries so

Comma before "so".

>+ * the caller should not use that space for value that must be preserved

s/value/values/

>+            if (!js_DefineNativeProperty(cx, obj, sprop->id,
>+                                         fp->spbase[slot], NULL, NULL,
>+                                         JSPROP_ENUMERATE | JSPROP_PERMANENT,
>+                                         SPROP_HAS_SHORTID, sprop->shortid,
>+                                         NULL)) {
>+                /*
>+                 * Assume failed js_DefineNativeProperty means out-of-memory
>+                 * or other quit-asap conditions.

Could be an exception thrown by clasp->addProperty -- can't we cope?

>+                 */
>+                normalUnwind = JS_FALSE;
>+                cx->throwing = JS_FALSE;
>+                goto out;
>+            }
>         }
>     }
> 
>-    return JS_SetPrivate(cx, obj, NULL);
>+  out:
>+    /* We must clear the private slot even with errors. */
>+    if (!JS_SetPrivate(cx, obj, NULL))

JS_SetPrivate is infallible -- we should stop testing it as we patch, make a clean sweep in a separate bug if necessary.

/be
Attached patch v6 (nit fixer) (obsolete) — Splinter Review
In the new version I addressed just the nits from the comment 10, it does not yet address:

> >+            if (!js_DefineNativeProperty(cx, obj, sprop->id,
> >+                                         fp->spbase[slot], NULL, NULL,
> >+                                         JSPROP_ENUMERATE | JSPROP_PERMANENT,
> >+                                         SPROP_HAS_SHORTID, sprop->shortid,
> >+                                         NULL)) {
> >+                /*
> >+                 * Assume failed js_DefineNativeProperty means out-of-memory
> >+                 * or other quit-asap conditions.
> 
> Could be an exception thrown by clasp->addProperty -- can't we cope?
Attachment #303178 - Attachment is obsolete: true
Attachment #303178 - Flags: review?(brendan)
Attached patch v5-v6 delta (obsolete) — Splinter Review
Attached patch v7 (obsolete) — Splinter Review
The new version fixes:

> >+                /*
> >+                 * Assume failed js_DefineNativeProperty means out-of-memory
> >+                 * or other quit-asap conditions.
> 
> Could be an exception thrown by clasp->addProperty -- can't we cope?

To properly detects that out-of-memory was thrown, not an exception that a script can catch, the patch clears cx->throwing in js_ReportOutOFMemeory and sets normalUnwind to false only if js_DefineNativeProperty failed with cx->throwing unset.

Here is a delta from v6:

--- .pc/.snap/js/src/jsobj.c	2008-02-15 00:50:12.000000000 +0100
+++ js/src/jsobj.c	2008-02-15 01:14:30.000000000 +0100
@@ -1942,18 +1942,19 @@ js_PutBlockObject(JSContext *cx, JSObjec
             if (!js_DefineNativeProperty(cx, obj, sprop->id,
                                          fp->spbase[slot], NULL, NULL,
                                          JSPROP_ENUMERATE | JSPROP_PERMANENT,
                                          SPROP_HAS_SHORTID, sprop->shortid,
                                          NULL)) {
                 /*
-                 * Assume failed js_DefineNativeProperty means out-of-memory
-                 * or other quit-asap conditions.
+                 * Stop adding properties if we failed due to out-of-memory or
+                 * other quit-asap errors.
                  */
-                normalUnwind = JS_FALSE;
-                cx->throwing = JS_FALSE;
-                goto out;
+                if (!cx->throwing) {
+                    normalUnwind = JS_FALSE;
+                    goto out;
+                }
             }
         }
     }
 
   out:
     /* We must clear the private slot even with errors. */
--- .pc/unwind.patch/js/src/jscntxt.c	2008-01-30 02:32:03.000000000 +0100
+++ js/src/jscntxt.c	2008-02-15 01:10:58.000000000 +0100
@@ -895,15 +895,18 @@ js_ReportOutOfMemory(JSContext *cx)
             report.lineno = js_PCToLineNumber(cx, fp->script, fp->pc);
             break;
         }
     }
 
     /*
-     * If debugErrorHook is present then we give it a chance to veto
-     * sending the error on to the regular ErrorReporter.
+     * If debugErrorHook is present then we give it a chance to veto sending
+     * the error on to the regular ErrorReporter. We also clear a pending
+     * exception if any now so the hooks can replace the out-of-memory error
+     * by a script-catchable exception.
      */
+    cx->throwing = JS_FALSE;
     if (onError) {
         JSDebugErrorHook hook = cx->debugHooks->debugErrorHook;
         if (hook &&
             !hook(cx, msg, &report, cx->debugHooks->debugErrorHookData)) {
             onError = NULL;
         }
Attachment #303399 - Attachment is obsolete: true
Attachment #303400 - Attachment is obsolete: true
Attachment #303405 - Flags: review?(brendan)
Attachment #303405 - Attachment is patch: true
Attachment #303405 - Attachment mime type: application/octet-stream → text/plain
Attachment #303399 - Attachment is patch: true
Attachment #303399 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 303405 [details] [diff] [review]
v7

I rewrote this:

>+         * EmitNonLocalJumpFixup may add fixup bytecode to close open try
>+         * blocks having finally clauses or to exit let blocks. We can't
>+         * simply transfer control flow to our caller in that case, because we
>+         * must gosub to those clauses from inner to outer, with the correct
>+         * stack pointer (i.e., after popping any with, for/in, etc., slots
>+         * nested inside the finally's try). In this case we mutate
>+         * JSOP_RETURN into JSOP_SETRVAL and add an extra JSOP_RETRVAL after
>+         * the fixups.

to this:

        /*
         * EmitNonLocalJumpFixup may add fixup bytecode to close open try
         * blocks having finally clauses and to exit intermingled let blocks.
         * We can't simply transfer control flow to our caller in that case,
         * because we must gosub to those finally clauses from inner to outer,
         * with the correct stack pointer (i.e., after popping any with,
         * for/in, etc., slots nested inside the finally's try).
         *
         * In this case we mutate JSOP_RETURN into JSOP_SETRVAL and add an
         * extra JSOP_RETRVAL after the fixups.
         */

Better? Use if so.

r/a=me, thanks for fixing this bug.

/be
Attachment #303405 - Flags: review?(brendan)
Attachment #303405 - Flags: review+
Attachment #303405 - Flags: approval1.9+
Attached patch v7bSplinter Review
Here is a version to check in with the fixed comment in jsemit.c.
Attachment #303405 - Attachment is obsolete: true
Attachment #303418 - Flags: review+
Attachment #303418 - Flags: approval1.9+
I checked in the version from comment 15 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1203075360&maxdate=1203075481&who=igor%25mir2.org
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
(In reply to comment #17)
> This patch regressed sunspider:

Here is the detailed data for the spider's tinderbox log:

03:07:26  788.84
03:25:10  795.98
03:38:01  782.38

the commit for this bug

03:56:48  765.34
04:10:55  759.26
05:01:42  793.52
05:27:21  790.24
	
The data is just too noisy to make any conclusions yet.
(In reply to comment #17)
> This patch regressed sunspider:
> 
> http://graphs.mozilla.org/graph.html#spst=range&spstart=0&spend=1203076814&bpst=cursor&bpstart=0&bpend=1203076814&m1tid=151880&m1bl=0&m1avg=0

The check-in happens at 2008-02-15 03:38, after whcih at least 2 points on the graph show no regression. The regression happens between 05:00 and 06:00, when, for example, the check in for bug 405128 happened.

As such I mark the bug fixed again.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
The patch didn't regress all sunspider tests equally. For example, a regression in access-fannkuch is pretty clear. Here's three runs before and after the patch:

access-fannkuch.html; 1381.5; 1376.5;  1365; 1385; 1365; 1378; 1385; 1380; 1383

access-fannkuch.html; 1367.5; 1363.25; 1357; 1384; 1369; 1361; 1366; 1384; 1357

access-fannkuch.html; 1381  ; 1371.75; 1358; 1386; 1367; 1383; 1386; 1358; 1379

-- patch --

access-fannkuch.html; 1470.5; 1446.75; 1419; 1529; 1419; 1511; 1430; 1427; 1529

access-fannkuch.html; 1514.5; 1465.25; 1414; 1520; 1418; 1414; 1518; 1520; 1511

access-fannkuch.html; 1516;   1467;    1415; 1552; 1421; 1415; 1552; 1516; 1516
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> 
> The check-in happens at 2008-02-15 03:38, after whcih at least 2 points on the
> graph show no regression. The regression happens between 05:00 and 06:00, 

I see the regression in a run that started at 03:54:00, on qm-pxp-fast01.
(In reply to comment #20)
> The patch didn't regress all sunspider tests equally. For example, a regression
> in access-fannkuch is pretty clear. Here's three runs before and after the
> patch:

I do not see a regression on Linux. Could you also apply the patch from bug 416439 on top of it? That bug targets code savings and needs some of code movements that this patch has done.
To Robert Sayre: could you try this patch on top of the patch in this bug to see if it would make a difference for you?
Yeah, I'll check this on windows, but it will take me a few hours.
(In reply to comment #24)
> Yeah, I'll check this on windows, but it will take me a few hours.
> 

I just realized, I don't have 2005pro, so I can't build with jemalloc. That might not matter, so I'll check anyway. Might be worth trying the patch on tinderbox.
(In reply to comment #25)
> (In reply to comment #24)
> > Yeah, I'll check this on windows, but it will take me a few hours.
> > 
> 
> I just realized, I don't have 2005pro, so I can't build with jemalloc. That
> might not matter, so I'll check anyway. Might be worth trying the patch on
> tinderbox.
> 

I checked this out--no difference, but unfortunately with a configuration not identical to the tinderbox.
(In reply to comment #26)
> I checked this out--no difference, but unfortunately with a configuration not
> identical to the tinderbox.

Still, could you try the patch from comment 23? And even if it would show no difference with MSVC, how would it affect the code size of jsinterp.o?
(In reply to comment #27)
> 
> Still, could you try the patch from comment 23? 

Tried the patch, but didn't see a difference.

> And even if it would show no
> difference with MSVC, how would it affect the code size of jsinterp.o?

jsinterp.obj: 235282 with patch
              235289 without patch

Comment on attachment 303418 [details] [diff] [review]
v7b

Forgot to suggest using

    RESTORE_SP(fp);

instead of the sevaral

    sp = fp->sp;

occurrences.

/be
Depends on: 417821
i filed bug 417821 to investigate the performance regression not to mix performance-related problems with fixing the original bug.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
/cvsroot/mozilla/js/tests/js1_7/regress/regress-416601.js,v  <--  regress-416601.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
v
Status: RESOLVED → VERIFIED
Blocks: 309894
No longer depends on: 309894
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: