Last Comment Bug 565157 - typed JS_AddRoot
: typed JS_AddRoot
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 578811
Blocks: fatvals
  Show dependency treegraph
 
Reported: 2010-05-11 14:32 PDT by Luke Wagner [:luke]
Modified: 2010-09-14 21:36 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (39.39 KB, patch)
2010-05-11 14:32 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
changes to js/src (22.27 KB, patch)
2010-05-13 17:07 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
mozilla changes (17.30 KB, patch)
2010-05-13 17:08 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
changes to js/src (20.26 KB, patch)
2010-06-04 14:39 PDT, Luke Wagner [:luke]
igor: review+
Details | Diff | Splinter Review
mozilla changes (19.79 KB, patch)
2010-06-04 14:40 PDT, Luke Wagner [:luke]
igor: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2010-05-11 14:32:01 PDT
Created attachment 444754 [details] [diff] [review]
patch

The 'void *' parameter of JS_AddRoot is a classic source of mistakes.  It's also a serious problem for fat values (bug 549143), since jsvals and gc-things will have different sizes and no distinguishing prefix.

This patch fixes both problems by splitting JS_(Add[Named]|Remove)Root into typed versions:

JSBool JS_AddValueRoot(JSContext *cx, jsval *vp);
JSBool JS_AddStringRoot(JSContext *cx, JSString **rp);
JSBool JS_AddObjectRoot(JSContext *cx, JSObject **rp);
JSBool JS_AddDoubleRoot(JSContext *cx, jsdouble **rp);
JSBool JS_AddGCThingRoot(JSContext *cx, void **rp);
JSBool JS_AddNamedValueRoot(JSContext *cx, jsval *vp, const char *name);
JSBool JS_AddNamedStringRoot(JSContext *cx, JSString **rp, const char *name);
JSBool JS_AddNamedObjectRoot(JSContext *cx, JSObject **rp, const char *name);
JSBool JS_AddNamedDoubleRoot(JSContext *cx, jsdouble **rp, const char *name);
JSBool JS_AddNamedGCThingRoot(JSContext *cx, void **rp, const char *name);
JSBool JS_AddNamedValueRootRT(JSRuntime *rt, jsval *vp, const char *name);
JSBool JS_AddNamedStringRootRT(JSRuntime *rt, JSString **rp, const char *name);
JSBool JS_AddNamedObjectRootRT(JSRuntime *rt, JSObject **rp, const char *name);
JSBool JS_AddNamedDoubleRootRT(JSRuntime *rt, jsdouble **rp, const char *name);
JSBool JS_AddNamedGCThingRootRT(JSRuntime *rt, void **rp, const char *name);
JSBool JS_RemoveValueRoot(JSContext *cx, jsval *vp);
JSBool JS_RemoveStringRoot(JSContext *cx, JSString **rp);
JSBool JS_RemoveObjectRoot(JSContext *cx, JSObject **rp);
JSBool JS_RemoveDoubleRoot(JSContext *cx, jsdouble **rp);
JSBool JS_RemoveGCThingRoot(JSContext *cx, void **rp);
JSBool JS_RemoveValueRootRT(JSRuntime *rt, jsval *vp);
JSBool JS_RemoveStringRootRT(JSRuntime *rt, JSString **rp);
JSBool JS_RemoveObjectRootRT(JSRuntime *rt, JSObject **rp);
JSBool JS_RemoveDoubleRootRT(JSRuntime *rt, jsdouble **rp);
JSBool JS_RemoveGCThingRootRT(JSRuntime *rt, void **rp);

Most uses in mozilla are easily updated.  The only annoying part is when wrappers were made for JS_AddRoot/JS_RemoveRoot that themselves take a void*.

Igor, let me know if I should assign an additional reviewer for the non-js changes.
Comment 1 Igor Bukanov 2010-05-11 22:43:08 PDT
(In reply to comment #0)
> Created an attachment (id=444754) [details]
> patch

The patch must keep both JS_AddRoot and JS_AddNamedRoot if we want compatibility. Also we do not need the RT version of the typed functions.

In the DEBUG builds JS_AddNamedRoot and JS_AddNamedRootRT should print a deprecation warning to stderr the first time they are used.  

> Igor, let me know if I should assign an additional reviewer for the non-js
> changes.

No, since that are straightforward changes AFAICS on the first glance. But please split patch in two: js/src/* changes and everything else.
Comment 2 Luke Wagner [:luke] 2010-05-12 10:17:44 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > Created an attachment (id=444754) [details] [details]
> > patch
> 
> The patch must keep both JS_AddRoot and JS_AddNamedRoot if we want
> compatibility.

We had discussed breaking compatibility on this; the fat value patch requires it: it cannot distinguish a gc-thing from a jsval.  The idea behind the patch is move away from the scary broken API earlier than later.

> Also we do not need the RT version of the typed functions.

The RT versions are used in mozilla.  It makes sense that one would want to add a root to the runtime, without having a particular context in hand.
Comment 3 Brendan Eich [:brendan] 2010-05-12 10:42:45 PDT
Right, we have to break. Do it now, the old API is just a trap. Please notify the js-engine group.

The rt-parameterized versions still make sense even with GC compartments, indeed.

/be
Comment 4 Wesley W. Garland 2010-05-12 12:51:03 PDT
Can we please have a C precompiler test to use, so that we can write code that works with spidermonkey releases both before and after this lands?
Comment 5 Luke Wagner [:luke] 2010-05-13 17:07:40 PDT
Created attachment 445236 [details] [diff] [review]
changes to js/src

Split the patch per comment 1.  Added #define JS_HAS_TYPED_ROOTING_API in jsapi.h per comment 4 (couldn't think of a better name, plus, Australian programmers must be used to this one by now).
Comment 6 Luke Wagner [:luke] 2010-05-13 17:08:08 PDT
Created attachment 445237 [details] [diff] [review]
mozilla changes
Comment 7 Nicholas Nethercote [:njn] 2010-05-13 19:33:28 PDT
(In reply to comment #5)
> Added #define JS_HAS_TYPED_ROOTING_API in
> jsapi.h per comment 4 (couldn't think of a better name, plus, Australian
> programmers must be used to this one by now).

"root", yes, but "rooting" still warrants a giggle, especially when its in all-caps.
Comment 8 Luke Wagner [:luke] 2010-05-13 21:51:46 PDT
JS_HAS_TYPED_ROOT_API?
Comment 9 Igor Bukanov 2010-05-14 06:50:42 PDT
(In reply to comment #2)
> The RT versions are used in mozilla.  It makes sense that one would want to add
> a root to the runtime, without having a particular context in hand.

The problem with the RT version is that they are explicitly allowed to be called outside of the request. We should really not proliferate them and explicitly require that any rooting happens explicitly inside a request with the exception that RemooveRoot can also happens during the finalization.
Comment 10 Luke Wagner [:luke] 2010-05-14 09:39:43 PDT
(In reply to comment #9)
> The problem with the RT version is that they are explicitly allowed to be
> called outside of the request. We should really not proliferate them and
> explicitly require that any rooting happens explicitly inside a request with
> the exception that RemooveRoot can also happens during the finalization.

That sounds like a separate bug.  This patch doesn't proliferate, it just maintains.
Comment 11 Igor Bukanov 2010-05-14 10:40:53 PDT
(In reply to comment #10)
> That sounds like a separate bug.  This patch doesn't proliferate, it just
> maintains.

It proliferates in sense of adding new functions RT functions as a replacement for single older one. If the users would be forced to update their code in any case we should also try to remove cases of the problematic usage. Thus I would prefer to remove the RT functions.

Granted, in few cases of their usage in mozilla codebase they are convenient as there is no trivially accessible JSContext instance to use. But in those cases the usage of JS_AddRoot is rather suboptimal and should be really replaced by one of the autorooters. If converting those case to the cx version of typed AddRoot functions is hard to do quickly, then at least we should just add the RT functions that are necessary to support mozilla's usage as JS_FRIEND_API to jsgc.h. We can remove that bad usage later in separated bug.
Comment 12 Igor Bukanov 2010-06-01 06:12:04 PDT
Luke: what is the status here? As I wrote above, we should try to avoid the RT functions unless it would be absolutely necessary for compatibility. Otherwise the patch is absolutely OK.
Comment 13 Luke Wagner [:luke] 2010-06-01 10:58:38 PDT
Taking out the RT calls isn't a very quick fix so it keeps getting preempted by other things, primarily fatvals.
Comment 14 Igor Bukanov 2010-06-01 11:16:47 PDT
(In reply to comment #13)
> Taking out the RT calls isn't a very quick fix so it keeps getting preempted by
> other things, primarily fatvals.

If it would help, just keep as friends those that necessary to build the browser. I will do in a followup their replacement with non-RT versions. And another observation is that in view of fatvals AddNamedDoubleRoot would be soon unnecessary.
Comment 15 Luke Wagner [:luke] 2010-06-04 14:39:57 PDT
Created attachment 449350 [details] [diff] [review]
changes to js/src

Now with the RT versions as friend APIs.
Comment 16 Luke Wagner [:luke] 2010-06-04 14:40:58 PDT
Created attachment 449351 [details] [diff] [review]
mozilla changes
Comment 17 Luke Wagner [:luke] 2010-06-04 14:47:09 PDT
Oops, I put the JS_FRIEND-fication changes in the mozilla-changes patch instead of the js/src-changes patch.
Comment 18 Igor Bukanov 2010-06-05 08:00:23 PDT
Comment on attachment 449350 [details] [diff] [review]
changes to js/src

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -1759,47 +1759,120 @@ JS_NewDoubleValue(JSContext *cx, jsdoubl
>+JS_PUBLIC_API(JSBool)
>+JS_AddStringRoot(JSContext *cx, JSString **rp)
>+{
>+    CHECK_REQUEST(cx);
>+    return js_AddGCThingRoot(cx, (void **)rp, NULL);
>+}

Let's use a union type as a parameter for js_AddRot and js_RemoveRoot to prevent aliasing warnings from GCC, with a usage like:

JS_AddStringRoot(JSContext *cx, JSString **rp)
{
    CHECK_REQUEST(cx);
    TypedRootPointer u;
    u.strp = rp;
    return js_AddGCThingRoot(cx, u, NULL);
}

This makes sure that GCC would not complain about aliasing warnings.

>+/*
>+ * This symbol may be used by embedders to detect the change from the old
>+ * JS_AddRoot(JSContext *, void *) APIs to the new ones above.
>+ */
>+#define JS_HAS_TYPED_ROOTING_API

Nit: rename this into JS_TYPED_ROOTING_API. JS_HAS is used for features that can be switched off during compilation. This is not the case here.

r+ with this fixed.
Comment 19 Luke Wagner [:luke] 2010-06-07 10:10:46 PDT
I'd rather not add the union.  There isn't a strict-aliasing hazard here to warn about, or even the appearance of one, and even if there was, the union wouldn't fix it.  The proposed union is for the *pointer* to the GC thing.  To actually improve anything, you would need a union U over JSString*, JSObject*, and double* and have js_AddRoot take a U*.  But then callers would need to cast from X** or void* to U* and so nothing is improved, but everything is a bit more verbose.
Comment 20 Igor Bukanov 2010-06-07 13:26:55 PDT
(In reply to comment #19)
> I'd rather not add the union. There isn't a strict-aliasing hazard here to
> warn about, or even the appearance of one, and even if there was, the union
> wouldn't fix it.  The proposed union is for the *pointer* to the GC thing.  To
> actually improve anything, you would need a union U over JSString*, JSObject*,
> and double* and have js_AddRoot take a U*.  But then callers would need to cast
> from X** or void* to U* and so nothing is improved, but everything is a bit
> more verbose.

Right, so just r+
Comment 21 Luke Wagner [:luke] 2010-06-07 21:06:00 PDT
http://hg.mozilla.org/tracemonkey/rev/2fc2a12a4565

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