946546 - Avoid extra allocation in JSCompartment::wrap(JSString*)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

David Major was looking over mochitest b-c and noticed that there are some large (~80KB) strings getting passed into JSCompartment::wrap. One was a data: URL for a Firefox logo. But some others were for session restore; those are probably pretty common during normal execution.

The issue is that the code [1] calls ensureLinear and then allocates a new string in the destination compartment. The first call to ensureLinear allocates, and then we allocate again in the dest compartment. It seems like we ought to be able to use copyNonPureChars on the source string if it's a rope and then use that resulting buffer for the dest string.

Does that sound reasonable, Luke? The usual rationale for ensureLinear is that we pay once for the allocation, and then any operations on the string after that will be faster (since operations on linear strings are faster than on ropes). But in this case I don't think the argument really holds much water since we have no evidence that the string will be used in the source compartment later on.

[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jscompartment.cpp#236

Comment 1

[Luke Wagner [:luke]]

Yes, that makes sense.  I guess what's special here is that we don't have to worry about hitting this N times in a row because we stick the string in a cache.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: copy JS strings directly into the destination compartment

Based on my limited understanding of JS strings, compartments, and the above
comments, here's a first cut at doing what Bill proposed.  This patch passes
'make check' on my machine and is currently running on try.

Bill, does this look about right to you?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8366245 [details] [diff] [review]
copy JS strings directly into the destination compartment

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

Thanks for doing this. Just needs a little fixing up, I think.

::: js/src/jscompartment.cpp
@@ +312,5 @@
> +     * allocating in source compartment), because we don't know whether the
> +     * flattening will pay off later.
> +     */
> +    ScopedJSFreePtr<jschar> copiedChars;
> +    if (!str->copyNonPureChars(cx, copiedChars))

Shoot, I just realized this probably won't work as-is. copyNonPureChars requires !hasPureChars(). I think you'll get assertions with this patch.

I think you actually need to do something like this:

if (str->hasPureChars()) {
  // call js_NewStringCopyN<CanGC>(cx, str->pureChars(), str->length()) sort of like the original code
} else {
  // do what your patch does
}

@@ -325,5 @@
> -         * compartment's wrapper map would be left dangling.
> -         */
> -        JSString *tmp = linear;
> -        MarkStringUnbarriered(&cx->runtime()->gcMarker, &tmp, "wrapped string");
> -        JS_ASSERT(tmp == linear);

We still need this hunk. Just change |linear| to |str|.

Comment 4

[Nathan Froyd [:froydnj]]

Thanks for the feedback.

(In reply to Bill McCloskey (:billm) from comment #3)
> ::: js/src/jscompartment.cpp
> @@ +312,5 @@
> > +     * allocating in source compartment), because we don't know whether the
> > +     * flattening will pay off later.
> > +     */
> > +    ScopedJSFreePtr<jschar> copiedChars;
> > +    if (!str->copyNonPureChars(cx, copiedChars))
> 
> Shoot, I just realized this probably won't work as-is. copyNonPureChars
> requires !hasPureChars(). I think you'll get assertions with this patch.

Oh yes.  The try push was very orange.

> @@ -325,5 @@
> > -         * compartment's wrapper map would be left dangling.
> > -         */
> > -        JSString *tmp = linear;
> > -        MarkStringUnbarriered(&cx->runtime()->gcMarker, &tmp, "wrapped string");
> > -        JS_ASSERT(tmp == linear);
> 
> We still need this hunk. Just change |linear| to |str|.

Why do we still need this hunk?  My understanding was that since linear was a temporary copy that we created in the source compartment, this hunk was addressing the necessity of dealing with that.  And we didn't do anything equivalent for the newly copied string in the destination compartment.  So why is dealing with the destination string necessary now?

Note that I'm not asking this because I believe you to be wrong!  I'm asking because I would like to understand what is going on here.

Comment 5

[Nathan Froyd [:froydnj]]

I went and wrote up the version that checks for pureChars:

https://tbpl.mozilla.org/?tree=Try&rev=a095df63112b

It did not work very well either.  We seem to be getting non-null terminated strings from pureChars()?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I suspect the lack of null termination is coming from copyNonPureChars. Luckily we can use copyNonPureCharsZ instead.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #6)
> I suspect the lack of null termination is coming from copyNonPureChars.
> Luckily we can use copyNonPureCharsZ instead.

I suppose ensuring that we had a linear string previously also silently guaranteed null termination?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nathan Froyd (:froydnj) from comment #7)
> I suppose ensuring that we had a linear string previously also silently
> guaranteed null termination?

Yes, the invariant for strings is that they end in a null character. It's only when we use copyNonPureChars that I could see us getting data that doesn't end in null.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: copy JS strings directly into the destination compartment

Looking at stuff in the debugger and String.h suggests that we need to handle the
hasPureCharsZ and hasPureChars cases separately.  This fixes the failures I see on
make check; I can't push to Try to confirm it fixes the mochitest failures right
now, though.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8367535 [details] [diff] [review]
copy JS strings directly into the destination compartment

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

::: js/src/jscompartment.cpp
@@ +312,5 @@
> +     * allocating in source compartment), because we don't know whether the
> +     * flattening will pay off later.
> +     */
> +    JSString *copy;
> +    if (str->hasPureCharsZ()) {

I don't think this case is useful. It should be subsumed by the hasPureChars case. The pureChars should work in all cases where pureCharsZ works. Also, js_NewStringCopyN doesn't require a null-terminated string; it copies into its own null-terminated buffer before creating the new JSString.

@@ +328,5 @@
>          return false;
>      if (!putWrapper(cx, key, StringValue(copy)))
>          return false;
>  
> +    if (copy->zone()->isGCMarking()) {

I'm having trouble remembering the details of why we need this code. However, I do know it should apply to |str|, not to |copy|. The basic idea is that an ongoing incremental GC might not have marked |str| at this point. If we end up not marking |str|, then we'll have an entry for |str| in the wrapper map even though |str| is dead. That's an error.

The part that I don't remember is why |str| might not be marked; the fact that we're passing it into wrap() suggests that it should be reachable. The only thing I can think of is that we're calling wrap() from within the VM on a string that isn't guaranteed to be alive for some reason.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jon, do you remember the details of why we need the isGCMarking stuff here? Please see my comment above this one.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(The code was added in bug 816046, which did appear to fix a real problem.)

Comment 13

[Jon Coppeard (:jonco)]

I agree that the original string should be reachable (and hence get marked) if we're wrapping it.

I didn't remember offhand why we did this, so I did some digging... 

I see that we later changed the wrap functions to call ExposeGCThingToActiveJS() on the target, in bug 853498.  This calls IncrementalReferenceBarrier() if the target's zone is being marked "since the gray bits haven't been computed yet".  So I think that this code is now unnecessary as we will already mark the target here, assuming the string came from outside the engine (and I can't see how it would not be marked otherwise).

The second thing that comes to mind is bug 905382 in the xpconnect string cache which might have been the source of unmarked strings in bug 816046.

Wherever they came from, I don't think this is necessary any more.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #10)
> Comment on attachment 8367535 [details] [diff] [review]
> copy JS strings directly into the destination compartment
> 
> Review of attachment 8367535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jscompartment.cpp
> @@ +312,5 @@
> > +     * allocating in source compartment), because we don't know whether the
> > +     * flattening will pay off later.
> > +     */
> > +    JSString *copy;
> > +    if (str->hasPureCharsZ()) {
> 
> I don't think this case is useful. It should be subsumed by the hasPureChars
> case. The pureChars should work in all cases where pureCharsZ works. Also,
> js_NewStringCopyN doesn't require a null-terminated string; it copies into
> its own null-terminated buffer before creating the new JSString.

It is demonstrably needed: https://tbpl.mozilla.org/?tree=Try&rev=a095df63112b

That changeset only checks for hasPureChars, and hits a number of asserts.  Though maybe that's just a flaw in the String code?  (Seems unlikely.)

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nathan Froyd (:froydnj) from comment #14)
> It is demonstrably needed:
> https://tbpl.mozilla.org/?tree=Try&rev=a095df63112b
> 
> That changeset only checks for hasPureChars, and hits a number of asserts. 
> Though maybe that's just a flaw in the String code?  (Seems unlikely.)

That push uses copyNonPureChars rather than copyNonPureCharsZ. I suspect that's why you're getting the assertion. If that's not the case, let me know and I'll try to reproduce the problem in a JS shell.

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #15)
> (In reply to Nathan Froyd (:froydnj) from comment #14)
> > It is demonstrably needed:
> > https://tbpl.mozilla.org/?tree=Try&rev=a095df63112b
> > 
> > That changeset only checks for hasPureChars, and hits a number of asserts. 
> > Though maybe that's just a flaw in the String code?  (Seems unlikely.)
> 
> That push uses copyNonPureChars rather than copyNonPureCharsZ. I suspect
> that's why you're getting the assertion. If that's not the case, let me know
> and I'll try to reproduce the problem in a JS shell.

Ah, that's the issue (at least JS shell tests don't fail locally).

I see now how this works: the previous code used js_NewStringCopyN, which implicitly null-terminates.  But the new code used js_NewString, which requires the provided string to be already null-terminated.

I have a try push running with copyNonPureCharsZ and the marking code removed.  I'll flag for review if that comes through green.  Thanks for your help in walking me through this.

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: copy JS strings directly into the destination compartment

Try is looking green-ish for the debug tests, so here's the patch for review.

Many thanks for helping out with this.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8368144 [details] [diff] [review]
copy JS strings directly into the destination compartment

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

Thanks for sticking with this.

Comment 19

[Nathan Froyd [:froydnj]]

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

Comment 20

[Carsten Book [:Tomcat]]

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