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

RESOLVED FIXED in Firefox 42

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla42
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8627480 [details] [diff] [review]
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.
Attachment #8627480 - Flags: review?(sphink)
Attachment #8627480 - Flags: review?(continuation)
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 ::
Attachment #8627480 - Flags: review?(continuation) → review+
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.
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
(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.
> 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.  ;)
(Assignee)

Comment 6

3 years ago
(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 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?
Attachment #8627480 - Flags: review?(sphink) → review+
(Assignee)

Comment 9

3 years ago
(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.
JS_GetEmptyString might be good enough, assuming that's actually an atom.
https://hg.mozilla.org/mozilla-central/rev/a849e759a0f7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
The JSAPI reference <https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference> needs updating to reflect these changes.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.