Do not allow DependentStrings to depend on InlineStrings

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #649451 - Flags: review?(luke)

Comment 1

5 years ago
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

5 years ago
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);
Attachment #649451 - Flags: review?(luke) → review+
(Assignee)

Comment 3

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

Doh!
(Assignee)

Comment 4

5 years ago
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
Attachment #649451 - Attachment is obsolete: true
Attachment #649471 - Flags: review+

Comment 5

5 years ago
Another option would be to dump JSDependentString::new_ in String-inl.h.
(Assignee)

Comment 6

5 years ago
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
Attachment #649471 - Attachment is obsolete: true
Attachment #649703 - Flags: review+
(Assignee)

Comment 7

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

Comment 8

5 years ago
Try looks green, pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e702ef9113
https://hg.mozilla.org/mozilla-central/rev/f4e702ef9113
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Depends on: 808834
You need to log in before you can comment on or make changes to this bug.