964238 - Make js_NewString() return static strings when appropriate.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I've been optimizing pdf.js. Scrolling through the first 50 pages of a
particular PDF -- which is actually a 4,125 page document -- causes over 2 MiB
of single-char, ASCII strings to be created.

Making them static strings saves memory and reduces GC pressure.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Make js_NewString() return static strings when appropriate.

I'm not 100% certain that the reinterpret_cast is ok...

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8365917 [details] [diff] [review]
Make js_NewString() return static strings when appropriate.

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

A second opinion here is probably a good idea...

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8365917 [details] [diff] [review]
Make js_NewString() return static strings when appropriate.

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

That's not going to work: the StableString invariant is !isInline and those 1-char strings are going to be inline. I'd prefer if you would just rip out StableString in entirety. We implemented aggressive pre-stabilization because it was the easiest implementation path to nursery-allocable-strings, not because it is the most efficient. At this point it looks like strings-in-the-nursery is not going to be a requirement for landing GGC, so I'd prefer to just re-think how a relocatable jschar* will work, since the existing code has caused so many memory problems. I was hoping to put this off until after GGCv1 landed, but if it needs to happen now we can probably make it happen. Unfortunately, I'm still in crunch time getting exact rooting stable for uplift, but hopefully after that I'll have a few cycles if you need the help. It should be pretty easy to rip out though: it's new enough that it doesn't really touch all that much.

Comment 4

[Nicholas Nethercote [inactive]]

Fair enough. This is only a minor improvement so it can wait.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove JSStableString and StableTwoByteChars.

This patch removes JSStableString and StableTwoByteChars. I replaced stable
strings with flat strings wherever possible.

It doesn't remove StableCharPtr. That class is similar to TwoByteCharsZ, but
the pointer type is |const jchar*| instead of |jschar*|, and there are more
constructors. As a result, replacing StableCharPtr with TwoByteCharsZ is
difficult. I'd be happy to rename it, though.

Note: in vm/String.h we have this comment line:

> *   Inline       0100       isFlat && !isExtensible && (u1.chars == inlineStorage) || isInt32)

The parentheses are ill-matched but I'm not sure what the correct expression
is.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make js_NewString() return static strings when appropriate.

That's better.

Comment 7

[Terrence Cole [:terrence]]

That should be:
isFlat && !isExtensible && ((u1.chars == inlineStorage) || isInt32)

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8366450 [details] [diff] [review]
(part 1) - Remove JSStableString and StableTwoByteChars.

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

That's a nice cleanup. r=me

::: js/src/json.cpp
@@ +809,5 @@
>                      : cx->names().undefined;
>      if (!str)
>          return false;
>  
> +    JSFlatString *flat = str->ensureFlat(cx);

Please make this Rooted, to be on the safe side.

::: js/src/vm/String-inl.h
@@ -39,5 @@
>  }
>  
>  template <AllowGC allowGC>
>  static JS_ALWAYS_INLINE JSInlineString *
> -NewShortString(ExclusiveContext *cx, JS::StableTwoByteChars chars)

I guess this hunk is part of the next patch?

::: js/src/vm/String.h
@@ -187,5 @@
>       *   Flat         -          isLinear && !isDependent
>       *   Undepended   0011       0011
>       *   Extensible   0010       0010
>       *   Inline       0100       isFlat && !isExtensible && (u1.chars == inlineStorage) || isInt32)
> -     *   Stable       0100       isFlat && !isExtensible && (u1.chars != inlineStorage)

And I guess this should have been: isFlat && !isExtensible && ((u1.chars != inlineStorage) || isInt32)

Comment 9

[Terrence Cole [:terrence]]

We should definitely fix the name of ShableCharPtr asap: it's a lie now and will just be more confusing cruft in spidermonkey unless we jump on it soon. I'm not sure what to call it though as TwoByteChars and TwoByteCharsZ are taken. Jeff, do you have any ideas/preferences here?

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8366452 [details] [diff] [review]
(part 2) - Make js_NewString() return static strings when appropriate.

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

Great! r=me

Comment 11

[Nicholas Nethercote [inactive]]

> We should definitely fix the name of ShableCharPtr asap: it's a lie now and
> will just be more confusing cruft in spidermonkey unless we jump on it soon.
> I'm not sure what to call it though as TwoByteChars and TwoByteCharsZ are
> taken.

ConstTwoByteCharsZ?

Comment 12

[Nicholas Nethercote [inactive]]

> > -NewShortString(ExclusiveContext *cx, JS::StableTwoByteChars chars)
> 
> I guess this hunk is part of the next patch?

No. There are multiple overloadings of NewShortString(), and this is the one that takes a StableTwoByteChars argument, and this patch removes that type.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove JSStableString and StableTwoByteChars.

Updated version. The new patch:

- Renames StableCharPtr as ConstTwoByteCharsZ, and adds a null-termination
  check, and moves it into CharacterEncoding.h.

- Renames CharPtr as ConstCharPtr.

- Fixes NewShortString((ExclusiveContext *cx, JS::TwoByteChars chars), which
  was wrong in the previous patch. This required me to also use
  js_NewStringCopyN<NoGC> in StaticStrings::init(), because using <CanGC> was
  causing some assertion failures relating to context exclusiveness.

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 8367009 [details] [diff] [review]
(part 1) - Remove JSStableString and StableTwoByteChars.

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

r=me

::: js/public/CharacterEncoding.h
@@ +151,5 @@
> +{
> +    typedef mozilla::RangedPtr<const jschar> ConstCharPtr;
> +
> +  public:
> +    // njn: check for Z

I guess you forgot to qref?

::: js/src/vm/String.h
@@ -657,5 @@
> -        return *this;
> -    }
> -
> -  private:
> -    JS::AutoStringRooter rooter;

This is the last AutoStringRooter, could you kill that class as well? I can make a follow-up if you just want to get this in and done with.

Comment 15

[Nicholas Nethercote [inactive]]

> > +    // njn: check for Z
> 
> I guess you forgot to qref?

No... just forgot to do it :)


> This is the last AutoStringRooter, could you kill that class as well?

It's still used in gc/RootMarking.cpp. Or can that case be removed? And likewise can the STRING value of the enum inside AutoGCRooter be removed?

Comment 16

[Terrence Cole [:terrence]]

(In reply to Nicholas Nethercote [:njn] from comment #15)
> 
> > This is the last AutoStringRooter, could you kill that class as well?
> 
> It's still used in gc/RootMarking.cpp. Or can that case be removed? And
> likewise can the STRING value of the enum inside AutoGCRooter be removed?

Yes and yes. The first is where we mark the AutoStringRooter instance in order to root it. For the second, the numbers are arbitrary as long as they are in -2**sizeof(ptrdiff_t)*8 < n < 0.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove AutoStringRooter, because it's no longer used.

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 8367526 [details] [diff] [review]
(part 3) - Remove AutoStringRooter, because it's no longer used.

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

Great! r=me

Comment 19

[Nicholas Nethercote [inactive]]

I added the null-termination checks to ConstTwoByteCharsZ... and hit failures. The first failure was in builtin/Eval.cpp, where we do this:

                JSONParser parser(cx, isArray ? chars : chars + 1U, isArray ? length : length - 2,
                                  JSONParser::NoError);

note that if |isArray| is false, we use |chars+1| and |length-2|.

Should I rename it to ConstTwoByteChars and just remove the null-termination checks?

Comment 20

[Terrence Cole [:terrence]]

Oh yeah, that monster. I had forgotten about that usage and been happier for it. I think this is why I needed StableCharPtr in the first place. I agree with your assessment: rename to remove the Z and leave out the null-termination checks.

Comment 21

[Nicholas Nethercote [inactive]]

I fixed a couple of problems that try server demonstrated:
- there was a missing root in CTypes.cpp
- js_NewString() was leaking when a static string was used -- it takes ownership of |chars|, and so needs to free it in that case.

I'm still gettting crashes in SM(r), though, e.g.:

  https://tbpl.mozilla.org/?tree=Try&rev=37fe7f3cf0dc

I'm having trouble reproducing them locally, unfortunately.

Comment 22

[Jeff Walden [:Waldo]]

Yeah, I'd have asked to remove Z/null-termination, so I think we're all on the same page.

Comment 23

[Nicholas Nethercote [inactive]]

I landed parts 1 and 3; part 2 is causing SM(r) crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f88ba0e5e3b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b6dfafd05c

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Make js_NewString() return static strings when appropriate.

Here's the latest version. Terrence, can you think why this might cause SM(r)
failures (in testParseJSON.cpp)?

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 8366452 [details] [diff] [review]
(part 2) - Make js_NewString() return static strings when appropriate.

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

::: js/src/jsstr.cpp
@@ +3957,5 @@
>  js_NewString(ThreadSafeContext *cx, jschar *chars, size_t length)
>  {
> +    if (length == 1 && StaticStrings::hasUnit(chars[0])) {
> +        return cx->staticStrings().getUnit(chars[0]);
> +    }

Whoops, missed this style nit initially: no braces around single line if.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f88ba0e5e3b1
https://hg.mozilla.org/mozilla-central/rev/19b6dfafd05c

Comment 27

[Nicholas Nethercote [inactive]]

Whoops, I forgot the [leave open] tag; one patch remains to be landed.

Comment 28

[Nicholas Nethercote [inactive]]

> Whoops, missed this style nit initially: no braces around single line if.

Well, that's changed anyway...

Comment 29

[Nicholas Nethercote [inactive]]

My latest try build was green (https://tbpl.mozilla.org/?tree=Try&rev=243d3bc568af) so let's try landing part 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/862474a812c7

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/862474a812c7