Latin1 strings: support dependent strings

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

To make the substr/substring/slice functions work.
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.
Attachment #8431696 - Flags: review?(luke)
Pretty straight-forward on top of the previous patch.
Attachment #8431704 - Flags: review?(luke)
Attachment #8431706 - Flags: review?(luke)
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 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.
Attachment #8431696 - Flags: review?(luke) → review+
Attachment #8431704 - Flags: review?(luke) → review+
Attachment #8431706 - Flags: review?(luke) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/bb9c83bfcee0
https://hg.mozilla.org/mozilla-central/rev/303e9291298b
https://hg.mozilla.org/mozilla-central/rev/66524f499e17
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.