1178581 - The usage of "interning" in the JSAPI is not correct

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: fix_interning_naming-v0.diff

Currently, we're using "intern" to mean atomize and hold live forever. Interning in no way implies infinite lifetime, only pointer stability for faster hash comparison, so this is just wrong. Really atomization should be interning and the current "interning" should be "intern + hold forever". Ideally, these two concepts would be totally orthogonal and we'd just require people to pin separately at the cost of a second hash or a list somewhere. For now though, I'd just like to start with just making the naming sane.

Since HoldJSFoo is already used in the browser, #jsapi consensus seemed to be Intern -> AtomizeAndPin, which is what I've done here. This is a simple rename and should be pretty trivial to review, despite the length.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 8627480 [details] [diff] [review]
fix_interning_naming-v0.diff

Review of attachment 8627480 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the browser changes

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +680,5 @@
>  doGetIdentifier(JSContext *cx, const NPUTF8* name)
>  {
>    NS_ConvertUTF8toUTF16 utf16name(name);
>  
> +  JSString *str = ::JS_AtomizeAndPinUCStringN(cx, utf16name.get(), utf16name.Length());

probably don't need the ::

Comment 2

[Boris Zbarsky [:bzbarsky]]

The comments in BindingUtils.cpp might want updating.

But really, what some of the DOM consumers want is a "get me a stable jsid from this string" operation; the fact that this involves some sort of atomizing or pinning on the JS engine side is an implementation detail that we'd just as soon not have to worry about.

Comment 3

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> The comments in BindingUtils.cpp might want updating.
> 
> But really, what some of the DOM consumers want is a "get me a stable jsid
> from this string" operation; the fact that this involves some sort of
> atomizing or pinning on the JS engine side is an implementation detail that
> we'd just as soon not have to worry about.

Awesome! That's basically what I was expecting via the comment in Id.h, I just wasn't sure how relevant the pinning half of interning is versus the atomization half. I guess we should consider giving most of the users proper, non-infinite rooting where appropriate. I think I saw at least one place where this would be better.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> The comments in BindingUtils.cpp might want updating.

Done! Thanks for the heads up.

Looking closer at the INTERNED_STRING_TO_ID code, it's just calling AtomToId, so the internals don't care /at all/ whether or not the string is pinned. This, of course, makes me very confused because it's no longer clear whether the original designer was punning interned with atomized, or whether it was a hack to "help" users of the API to "root" jsids before we had proper rooting primitives for that.

Uhg. In any case, I'll add |setPinnedAtom| and |setAtom| to jsid; add |PinnedAtomId| and |AtomId| to Id.h; and make AtomToId share code as appropriate. I guess we won't have many users of the non-pinned side initially, but at least it will give us a smooth upgrade path.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> or whether it was a hack to "help" users of the API to "root" jsids before we had proper
> rooting primitives for that

Probably the latter...

For what it's worth, for bindings stuff we really do want the ids to stick around forever once we create them; we can do that via pinning or we could try to do some sort of persistentrooted type thing, I guess.  Anyway, making it clearer what's really going on here (creation of persistent jsids) would be good.  ;)

Comment 6

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> > or whether it was a hack to "help" users of the API to "root" jsids before we had proper
> > rooting primitives for that
> 
> Probably the latter...
> 
> For what it's worth, for bindings stuff we really do want the ids to stick
> around forever once we create them; we can do that via pinning or we could
> try to do some sort of persistentrooted type thing, I guess.  Anyway, making
> it clearer what's really going on here (creation of persistent jsids) would
> be good.  ;)

In that case, a bit on the table is probably fine, given that it is less memory and already exists. I think I want to make jsid only care about atomness, now that we have thorough primitives for rooting jsid that are independent from pinning. It will avoid a ton of confusion about what API needs to be used.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8627480 [details] [diff] [review]
fix_interning_naming-v0.diff

Review of attachment 8627480 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +4089,5 @@
>   * fail. As indicated by the lack of a JSContext parameter, there are two
>   * special cases where getting the chars is infallible:
>   *
> + * The first case is for strings that have been atomized, e.g. incidentally by
> + * JS_AtomizeAndPinString or explicitly because it is used in a jsid.

This comment sounds backwards. Calling JS_AtomizeAndPinString seems like a pretty explicit way to atomize something to me, and using something as a jsid only implicitly implies that it was atomized. (?)

::: js/xpconnect/src/ExportHelpers.cpp
@@ +435,5 @@
>              // copy the name from the function being imported.
>              JSFunction* fun = JS_GetObjectFunction(funObj);
>              RootedString funName(cx, JS_GetFunctionId(fun));
>              if (!funName)
> +                funName = JS_AtomizeAndPinString(cx, "");

Couldn't this use cx->names().empty instead of creating a new string?

Comment 8

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b3ad9674fb4&filter-tier=1

Comment 9

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #7)
> Comment on attachment 8627480 [details] [diff] [review]
> fix_interning_naming-v0.diff
> 
> Review of attachment 8627480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/src/ExportHelpers.cpp
> @@ +435,5 @@
> >              // copy the name from the function being imported.
> >              JSFunction* fun = JS_GetObjectFunction(funObj);
> >              RootedString funName(cx, JS_GetFunctionId(fun));
> >              if (!funName)
> > +                funName = JS_AtomizeAndPinString(cx, "");
> 
> Couldn't this use cx->names().empty instead of creating a new string?

Probably, but I'm not sure how to get at it easily from XPConnect, or if this one case is worth adding a new entry point specially for it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

JS_GetEmptyString might be good enough, assuming that's actually an atom.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a849e759a0f7

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a849e759a0f7

Comment 13

[Myk Melez [:myk] [@mykmelez]]

The JSAPI reference <https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference> needs updating to reflect these changes.