Last Comment Bug 780765 - Do not allow DependentStrings to depend on InlineStrings
: Do not allow DependentStrings to depend on InlineStrings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
: general
Mentors:
Depends on: 808834
Blocks: ExactRooting
  Show dependency treegraph
 
Reported: 2012-08-06 16:04 PDT by Terrence Cole [:terrence]
Modified: 2012-11-05 16:03 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (3.61 KB, patch)
2012-08-06 16:04 PDT, Terrence Cole [:terrence]
luke: review+
Details | Diff | Splinter Review
v1 (3.18 KB, patch)
2012-08-06 16:41 PDT, Terrence Cole [:terrence]
terrence: review+
Details | Diff | Splinter Review
vFinal: fixed all nits and moved NewShortString up. (6.40 KB, patch)
2012-08-07 10:54 PDT, Terrence Cole [:terrence]
terrence: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-08-06 16:04:02 PDT
Created attachment 649451 [details] [diff] [review]
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 2012-08-06 16:10:01 PDT
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] 2012-08-06 16:20:41 PDT
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] 2012-08-06 16:33:33 PDT
(In reply to Benjamin Peterson from comment #1)
> Instead of this whole block. Can you use NewShortString?

Doh!
Comment 4 Terrence Cole [:terrence] 2012-08-06 16:41:48 PDT
Created attachment 649471 [details] [diff] [review]
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 2012-08-06 16:49:11 PDT
Another option would be to dump JSDependentString::new_ in String-inl.h.
Comment 6 Terrence Cole [:terrence] 2012-08-07 10:54:55 PDT
Created attachment 649703 [details] [diff] [review]
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] 2012-08-07 13:52:59 PDT
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] 2012-08-08 11:05:03 PDT
Try looks green, pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e702ef9113
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-08 18:24:46 PDT
https://hg.mozilla.org/mozilla-central/rev/f4e702ef9113

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