1018311 - Latin1 strings: support dependent strings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

To make the substr/substring/slice functions work.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Refactoring

Refactors things to pass the start index instead of the jschar* pointer to JSDependentString::new_ and JSDependentString::init. The char pointer is no longer live across calls that can GC.

This will work nicely with strings in the nursery and makes supporting latin1 in the next patch a bit simpler.

At some point we'll probably need a "rooted chars" type, but I want to see how far we can get without it. In many cases a JSString + index works better with latin1 strings anyway.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Support latin1 strings

Pretty straight-forward on top of the previous patch.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Tests

Comment 4

[Jan de Mooij [:jandem]]

Btw, when we create a (fat) inline string instead of a dependent string, we could deflate to Latin1 if possible, so that we can store more characters inline. We've discussed doing the same for TwoByte strings passed in from Gecko.

It should be easy to measure how much this helps once we get Latin1 strings working in the browser.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8431696 [details] [diff] [review]
Part 1 - Refactoring

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

::: js/src/jsstr.cpp
@@ +4004,5 @@
>  
> +    {
> +        AutoCheckCannotGC nogc;
> +        const jschar *chars = base->twoByteChars(nogc) + start;
> +        if (JSLinearString *staticStr = cx->staticStrings().lookup(chars, length))

We'll be able to switch all the static strings to Latin1, right?

::: js/src/vm/String-inl.h
@@ +151,5 @@
>  }
>  
>  MOZ_ALWAYS_INLINE JSLinearString *
>  JSDependentString::new_(js::ExclusiveContext *cx,
> +                        JSLinearString *baseArg, size_t start, size_t length)

Pre-existing, but I think it'd look better to put as many of the arguments on the first line.

@@ +187,4 @@
>          return str;
>      }
>  
>      JS::Rooted<JSLinearString*> base(cx, baseArg);

I wonder if it's really worth saving a Rooted to do this NoGC-then-CanGC dance.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9c83bfcee0
https://hg.mozilla.org/integration/mozilla-inbound/rev/303e9291298b
https://hg.mozilla.org/integration/mozilla-inbound/rev/66524f499e17

(In reply to Luke Wagner [:luke] from comment #5)
> We'll be able to switch all the static strings to Latin1, right?

I think so, yes.

> Pre-existing, but I think it'd look better to put as many of the arguments
> on the first line.

Done.

> I wonder if it's really worth saving a Rooted to do this NoGC-then-CanGC
> dance.

Yeah, I was wondering the same. I left it as it doesn't really affect my changes.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb9c83bfcee0
https://hg.mozilla.org/mozilla-central/rev/303e9291298b
https://hg.mozilla.org/mozilla-central/rev/66524f499e17