780765 - Do not allow DependentStrings to depend on InlineStrings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

These should just store the chars inline anyway, and we can avoid a nasty moving GC hazard by doing so.

Comment 1

[:Benjamin Peterson]

Comment on attachment 649451 [details] [diff] [review]
v0

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

::: js/src/vm/String-inl.h
@@ +132,3 @@
>       */
> +    if (base->isInline()) {
> +        JS::SkipRoot charsRoot(cx, &chars);

Instead of this whole block. Can you use NewShortString?

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 649451 [details] [diff] [review]
v0

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

Could you also add a JS_ASSERT(!d.s.u2.base->isInlineString()) to JSString::base()?

::: js/src/vm/String-inl.h
@@ +132,2 @@
>       */
> +    if (base->isInline()) {

As the String.h comment points out: isInline() isn't a very fast query, since it ends up looking at the arena kind in the case of short strings.  Could we instead just write:

  if (JSShortString::lengthFits(base->length()))

@@ +145,5 @@
> +        if (!str)
> +            return NULL;
> +        str->initAtOffsetInBuffer(str->inlineStorageBeforeInit(), length);
> +        js::PodCopy((jschar *)str->chars(), chars, length);
> +        return str;

I think you can just write:

  if (base->isInline())
      return NewShortString(cx, chars, length);

Comment 3

[Terrence Cole [:terrence]]

(In reply to Benjamin Peterson from comment #1)
> Instead of this whole block. Can you use NewShortString?

Doh!

Comment 4

[Terrence Cole [:terrence]]

Attachment: v1

Well, I had to forward-declare NewShortString, but it's still less gross than accidentally inlining it. :-)

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

Comment 5

[:Benjamin Peterson]

Another option would be to dump JSDependentString::new_ in String-inl.h.

Comment 6

[Terrence Cole [:terrence]]

Attachment: vFinal: fixed all nits and moved NewShortString up.

Well, it's all already in vm/String-inl.h.  I have moved NewShortString fully to the top of the file rather than forward declaring it.  It doesn't seem to have depended on anything above it.

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

Comment 7

[Terrence Cole [:terrence]]

The last try run seems broken on windows and mac for unrelated reasons:
https://tbpl.mozilla.org/?tree=Try&rev=e28a7f56bbdb

Comment 8

[Terrence Cole [:terrence]]

Try looks green, pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e702ef9113

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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