Make js_NewString() return static strings when appropriate.

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 attachments, 3 obsolete attachments)

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.
I'm not 100% certain that the reinterpret_cast is ok...
Attachment #8365917 - Flags: review?(luke)
Blocks: 881974
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...
Attachment #8365917 - Flags: review?(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.
Attachment #8365917 - Flags: review?(terrence) → review-
Fair enough. This is only a minor improvement so it can wait.
Whiteboard: [MemShrink]
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.
Attachment #8366450 - Flags: review?(terrence)
That's better.
Attachment #8366452 - Flags: review?(terrence)
Attachment #8365917 - Attachment is obsolete: true
Attachment #8365917 - Flags: review?(luke)
That should be:
isFlat && !isExtensible && ((u1.chars == inlineStorage) || isInt32)
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)
Attachment #8366450 - Flags: review?(terrence) → review+
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?
Flags: needinfo?(jwalden+bmo)
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
Attachment #8366452 - Flags: review?(terrence) → review+
> 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?
> > -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.
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.
Attachment #8367009 - Flags: review?(terrence)
Attachment #8366450 - Attachment is obsolete: true
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.
Attachment #8367009 - Flags: review?(terrence) → review+
> > +    // 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?
(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 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
Attachment #8367526 - Flags: review?(terrence) → review+
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?
Flags: needinfo?(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.
Flags: needinfo?(terrence)
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.
Flags: needinfo?(jwalden+bmo)
Yeah, I'd have asked to remove Z/null-termination, so I think we're all on the same page.
Here's the latest version. Terrence, can you think why this might cause SM(r)
failures (in testParseJSON.cpp)?
Attachment #8366452 - Attachment is obsolete: true
Flags: needinfo?(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.
https://hg.mozilla.org/mozilla-central/rev/f88ba0e5e3b1
https://hg.mozilla.org/mozilla-central/rev/19b6dfafd05c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Whoops, I forgot the [leave open] tag; one patch remains to be landed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [MemShrink] → [MemShrink][leave open]
> Whoops, missed this style nit initially: no braces around single line if.

Well, that's changed anyway...
Whiteboard: [MemShrink][leave open] → [MemShrink:P2][leave open]
Flags: needinfo?(terrence)
Whiteboard: [MemShrink:P2][leave open] → [MemShrink:P2]
https://hg.mozilla.org/mozilla-central/rev/862474a812c7
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.