Last Comment Bug 357169 - GC temp roots cleanup
: GC temp roots cleanup
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 358528 358661 358867
Blocks: js1.7src
  Show dependency treegraph
 
Reported: 2006-10-18 12:26 PDT by Igor Bukanov
Modified: 2006-11-29 19:00 PST (History)
6 users (show)
jaymoz: blocking1.8.1.1-
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work in progress (19.80 KB, patch)
2006-10-18 12:34 PDT, Igor Bukanov
no flags Details | Diff | Review
Implementation v1 (36.02 KB, patch)
2006-10-19 07:54 PDT, Igor Bukanov
no flags Details | Diff | Review
Implementation v2 (31.67 KB, patch)
2006-10-26 15:31 PDT, Igor Bukanov
brendan: review+
Details | Diff | Review
Implementation v2b (31.69 KB, patch)
2006-10-26 16:23 PDT, Igor Bukanov
igor: review+
Details | Diff | Review
Implementation v12b + fix from bug 358528 (23.09 KB, patch)
2006-10-28 13:04 PDT, Igor Bukanov
jaymoz: approval1.8.1.1+
Details | Diff | Review
simple patch (776 bytes, patch)
2006-11-01 02:23 PST, Evan Yan
no flags Details | Diff | Review
Implementation v12b + fix from bug 358528 + fix from bug 358867 (31.86 KB, patch)
2006-11-27 17:52 PST, Igor Bukanov
jaymoz: approval1.8.1.1+
Details | Diff | Review

Description Igor Bukanov 2006-10-18 12:26:07 PDT
Currently JS_PUSH_SINGLE_TEMP_ROOT macro casts its argument to jsval to allow to root arbitrary GC thing. But this may hide a bugs when improper value is passed to the macro. Thus the idea is to add JS_PUSH_TEMP_ROOT_STRING and JS_PUSH_TEMP_ROOT_GCTHING and remove the cast from JS_PUSH_SINGLE_TEMP_ROOT.
Comment 1 Igor Bukanov 2006-10-18 12:34:46 PDT
Created attachment 242671 [details] [diff] [review]
Work in progress

The patch adds the new macros, switches AddRoot/RemoveRoot pair from jsscan.c to use temp root instead and moves implementation of root mapping and dumping to jsgc.c not to leak too much implementation details from jsgc.c
Comment 2 Igor Bukanov 2006-10-19 07:54:25 PDT
Created attachment 242764 [details] [diff] [review]
Implementation v1

Compared with the previous patch this version switches js_ReportUncaughtException to use JSTempValueRooter instead of AllocStack for local roots and removes  
JSGCLockHashEntry and JSGCRootHashEntry declarations from jsprvtd.h. 

Another chunk of changes comes from movement of InitGC/FinishGC after AddRoot/RemoveRoot and related methods to avoid forward declarations.
Comment 3 Brendan Eich [:brendan] 2006-10-24 05:04:30 PDT
Comment on attachment 242764 [details] [diff] [review]
Implementation v1

>+ * JS_PUSH_TEMP_ROOT_OBJECT and JS_PUSH_TEMP_ROOT_STRING are type-safe
>+ * alternatives to JS_PUSH_TEMP_ROOT_GCTHING for JSObject * and JSString *.

"... for JSObject and JSString roots."

>+ * They also provide a simple way to get a single pointer to rooted JSObject *
>+ * or JSString * via  JS_PUSH_TEMP_ROOT_OBJECT(cx, NULL, &tvr) or

Don't need the * after JSObject and JSString above to add a level of indirection.

>+ * JS_PUSH_TEMP_ROOT_STRING(cx, NULL, &tvr). Then &tvr.u.object or
>+ * tvr.u.string give the necessary pointer, which puns tvr.u.value safely
>+ * because JSObject * and JSString * are GC-things and as such their tag bits

"are GC-thing pointers, and as such ...."

>diff -U7 -p -r3.179 jsgc.c
>--- jsgc.c	7 Oct 2006 19:14:55 -0000	3.179
>+++ jsgc.c	19 Oct 2006 14:46:50 -0000
>@@ -586,14 +586,208 @@ js_ChangeExternalStringFinalizer(JSStrin
>         if (gc_finalizers[i] == (GCFinalizeOp) oldop) {
>             gc_finalizers[i] = (GCFinalizeOp) newop;
>             return (intN) i;
>         }
>     }
>     return -1;
> }
>+/* This is compatible with JSDHashEntryStub. */

Need a blank line before the comment above. Also, is this all moved only because of CheckLeakedRoots?  Might be nicer to leave the root hash stuff where it was and forward-declare that static helper.

>+++ jsscan.c	19 Oct 2006 14:46:54 -0000
>@@ -527,14 +527,15 @@ MatchChar(JSTokenStream *ts, int32 expec
> }
> 
> static JSBool
> ReportCompileErrorNumber(JSContext *cx, void *handle, uintN flags,
>                          uintN errorNumber, JSErrorReport *report,
>                          JSBool charArgs, va_list ap)
> {
>+    JSTempValueRooter lineTvr;

Nit in light of other local names (mostly linestr; onError is contrary, granted): linetvr would be more consistent with local names such as linestr.

Looks great otherwise, thanks.

/be
Comment 4 Igor Bukanov 2006-10-26 15:31:32 PDT
Created attachment 243716 [details] [diff] [review]
Implementation v2

Addressing the nits and minimizing code movements.
Comment 5 Brendan Eich [:brendan] 2006-10-26 15:51:48 PDT
Comment on attachment 243716 [details] [diff] [review]
Implementation v2


>+ * JS_PUSH_TEMP_ROOT_OBJECT and JS_PUSH_TEMP_ROOT_STRING are type-safe
>+ * alternatives to JS_PUSH_TEMP_ROOT_GCTHING for JSObject and JSString. They
>+ * also provide a simple way to get a single pointer to rooted JSObject or
>+ * JSString via  JS_PUSH_TEMP_ROOT_OBJECT(cx, NULL, &tvr) or
>+ * JS_PUSH_TEMP_ROOT_STRING(cx, NULL, &tvr). Then &tvr.u.object or
>+ * tvr.u.string give the necessary pointer, which puns tvr.u.value safely
>+ * because JSObject * and JSString * are GC-things and, as such their tag bits
>  * are all zeroes.

Final nit: wrap to shorter tw than 78?  Anyway, lose the extra space before
JS_PUSH_TEMP_ROOT_OBJECT(cx, NULL, &tvr), and s/give/gives/.

r=me, thanks.

/be
Comment 6 Igor Bukanov 2006-10-26 16:23:03 PDT
Created attachment 243729 [details] [diff] [review]
Implementation v2b

Patch to commit with comments reformatted.
Comment 7 Igor Bukanov 2006-10-27 12:39:49 PDT
I committed the patch from comment 6 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.285; previous revision: 3.284
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.99; previous revision: 3.98
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.124; previous revision: 3.123
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.80; previous revision: 3.79
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.170; previous revision: 3.169
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.180; previous revision: 3.179
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.59; previous revision: 3.58
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.300; previous revision: 3.299
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.299; previous revision: 3.298
done
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.21; previous revision: 3.20
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.127; previous revision: 3.126
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.116; previous revision: 3.115
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.135; previous revision: 3.134
done
Comment 8 Igor Bukanov 2006-10-28 13:04:17 PDT
Created attachment 243914 [details] [diff] [review]
Implementation v12b + fix from bug 358528

This is a patch for 1.8.1 branch that includes the fix for the regression reported in bug 358528.
Comment 9 Brendan Eich [:brendan] 2006-10-28 16:22:53 PDT
How'd I miss that?  Skimming, sorry.

/be
Comment 10 Evan Yan 2006-11-01 02:21:34 PST
Sun compiler complains:

"../../../mozilla/js/src/jsapi.c", line 1805: void function cannot return value cc: acomp failed for ../../../mozilla/js/src/jsapi.c 
Comment 11 Evan Yan 2006-11-01 02:23:03 PST
Created attachment 244294 [details] [diff] [review]
simple patch
Comment 12 Igor Bukanov 2006-11-01 05:05:42 PST
(In reply to comment #11)
> Created an attachment (id=244294) [edit]
> simple patch

This should go to bug 358867 as regression where Brendan already created a patch.

Comment 13 Brendan Eich [:brendan] 2006-11-01 10:48:08 PST
(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=244294) [edit]
> > simple patch
> 
> This should go to bug 358867 as regression where Brendan already created a
> patch.

Alfred did.

Evan: just to be clear, REOPENing a bug is best done when the bug was not in fact fixed by the patch that landed.  A further problem introduced by the patch, or any other recurrence that shows an unpatched bug, should be filed anew.

/be
Comment 14 Evan Yan 2006-11-01 17:20:54 PST
clear. thanks for explanation.
Comment 15 Daniel Veditz [:dveditz] 2006-11-20 12:06:25 PST
Brendan: you nominated this for blocking, but it's severity is set to "enhancement". Do we really need this in 2.0.0.1?
Comment 16 Brendan Eich [:brendan] 2006-11-20 12:32:19 PST
Igor can say whether this is causing pain merging trunk fixes that are wanted.  We want this patch for js1.7src, but we could mini-branch if we have to (rather match 1.8.1.1 exactly).

If you ignore all the DEBUG-related root-dumping changes, it's a small patch, which improves clarity and efficiency.  I think you should take it.

/be

Comment 17 Jay Patel [:jay] 2006-11-21 12:54:04 PST
Comment on attachment 243914 [details] [diff] [review]
Implementation v12b + fix from bug 358528

Approved for 1.8.1 branch, a=jay for drivers.  Please land asap.  Thanks!
Comment 18 Igor Bukanov 2006-11-22 00:43:13 PST
Bug 358528 is a regression so it must depend on this bug, not block the bug.
Comment 19 Igor Bukanov 2006-11-27 17:49:25 PST
Bug 358867 is debug-only compiler-only regression that I missed to include into the blocking list. 
Comment 20 Igor Bukanov 2006-11-27 17:52:39 PST
Created attachment 246739 [details] [diff] [review]
Implementation v12b + fix from bug 358528 + fix from bug 358867

The missed dependency forces asking one more approval.
Comment 21 Jay Patel [:jay] 2006-11-27 17:57:06 PST
Comment on attachment 246739 [details] [diff] [review]
Implementation v12b + fix from bug 358528 + fix from bug 358867

Approved for 1.8.1 branch, a=jay for drivers.
Comment 22 Daniel Veditz [:dveditz] 2006-11-27 17:57:15 PST
Believe it or not, we make regressions -block- the original bug rather than the other way around: to correctly fix this bug you really need to fix the others too.

Helps in trying to land bugs on old branches.
Comment 23 Igor Bukanov 2006-11-27 18:11:49 PST
I committed the patch from comment 20 to MOZILLA_1_8_BRANCH: 

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.214.2.31; previous revision: 3.214.2.30
done
Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.58.2.23; previous revision: 3.58.2.22
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.80.4.19; previous revision: 3.80.4.18
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.12; previous revision: 3.50.4.11
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.117.2.29; previous revision: 3.117.2.28
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.29; previous revision: 3.104.2.28
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.33.4.10; previous revision: 3.33.4.9
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.76; previous revision: 3.181.2.75
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.45; previous revision: 3.208.2.44
done
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.17.20.3; previous revision: 3.17.20.2
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.99.2.19; previous revision: 3.99.2.18
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.81.2.27; previous revision: 3.81.2.26
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.57; previous revision: 3.50.2.56
done
Comment 24 Jay Patel [:jay] 2006-11-29 17:50:32 PST
This appears to have caused regression bug 358661.

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