Closed Bug 1403911 Opened 2 years ago Closed 2 years ago

Various clean-ups for jsstr.h/cpp

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(16 files, 6 obsolete files)

1.65 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.82 KB, patch
jandem
: review+
Details | Diff | Splinter Review
7.34 KB, patch
jandem
: review+
Details | Diff | Splinter Review
994 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
1.51 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.69 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.54 KB, patch
jandem
: review+
Details | Diff | Splinter Review
5.30 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.60 KB, patch
jandem
: review+
Details | Diff | Splinter Review
12.70 KB, patch
jandem
: review+
Details | Diff | Splinter Review
4.74 KB, patch
anba
: review+
Details | Diff | Splinter Review
7.54 KB, patch
anba
: review+
Details | Diff | Splinter Review
9.61 KB, patch
anba
: review+
Details | Diff | Splinter Review
8.67 KB, patch
anba
: review+
Details | Diff | Splinter Review
6.13 KB, patch
anba
: review+
Details | Diff | Splinter Review
9.52 KB, patch
anba
: review+
Details | Diff | Splinter Review
I have a couple of patches in my queue to clean-up and give slight performance improvements to string methods.
Part 1:
- Replace hand-written implementation of js_strlen with std::char_traits::length, added MOZ_ALWAYS_INLINE to ensure the compilers won't emit two calls (one to js_strlen and then another one to std::char_traits::length).
- js_strcmp was only used in two test files, both uses can be replaced with EqualChars.
- Remove unused js_strncpy function.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8913275 - Flags: review?(jdemooij)
Part 2:
- Improves the "long string" test case from bug 551539 from 100ms to 5ms for me.
- Looks like the performance of memcmp is no longer sad on Linux! :-)
Attachment #8913276 - Flags: review?(jdemooij)
Part 3:
- Only removes unnecessary rooting.
Attachment #8913277 - Flags: review?(jdemooij)
Part 4:
- IsRegExp also tests for objects, but since IsRegExp is not inlined in gcc/clang, we always have to pay the cost of an additional function call.
- Improves this µ-benchmark from ~200ms to ~175ms for me:
    var s = ["AABAA", "BBABB"];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 5000000; ++i) {
        q += s[i & 1].startsWith("C") ? 1 : 0;
    }
    print(dateNow() - t);
Attachment #8913278 - Flags: review?(jdemooij)
Part 5:
- Except for str_escape, all other functions were already rooting the return value from ArgToRootedString, so it doesn't seem useful to keep the "root-argument-in-arg-vector" functionality in ArgToRootedString.
- Also renamed the function to ArgToLinearString to indicate it no longer roots the string.
Attachment #8913279 - Flags: review?(jdemooij)
Part 6:
- Atomizing was a left-over before the ES6 RegExp changes were implemented.
Attachment #8913280 - Flags: review?(jdemooij)
Part 7:
- Fixes some style nits.
Attachment #8913281 - Flags: review?(jdemooij)
Part 8:
- There was nearly identical code in BuildDollarReplacement and BuildFlatReplacement to splice the replacement text into the matched string, so to avoid code duplication all string merging is now moved to BuildFlatReplacement (and InterpretDollarReplacement only performs the $-substitution, but doesn't concat the three string parts).
- The rope-specialization is now only performed in BuildFlatRopeReplacement (this looks a bit messy in the diff, because of the reindentation).
- And we can avoid some extra rooting and calls to ConcatStrings in BuildFlatReplacement if we manually call ConcatStrings instead of going through RopeBuilder. 

On top of the other changes, it improves this µ-benchmark from 400ms to 350ms for me:

    var s = ["a", "b"];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 5000000; ++i) {
        q += s[i&1].replace(s[i&1], "A").length;
    }
    print(dateNow() - t);

And this one improves from ~760-780ms to ~700-720ms:
    
    var s = ["a", "b"];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 5000000; ++i) {
        q += s[i&1].replace(s[i&1], "$&").length;
    }
    print(dateNow() - t);
Attachment #8913282 - Flags: review?(jdemooij)
Part 9:
- The out-param is no longer used by its callers.
Attachment #8913283 - Flags: review?(jdemooij)
Part 10:
- This is probably an unlikely case, but it's easy enough to make it a bit faster using MaybeHasInterestingSymbolProperty.

On top of the other changes, it improves this µ-benchmark from 660ms to 460ms for me:

    var s = [new String("a"), new String("b")];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 5000000; ++i) {
        q += s[i&1].trim().length;
    }
    print(dateNow() - t);
Attachment #8913284 - Flags: review?(jdemooij)
Part 11:
- Simply because CallSelfHostedFunction requires fewer lines of code to call a self-hosted function.
Attachment #8913285 - Flags: review?(jdemooij)
Part 12:
- That way we only need to set |char16_t* invalidFlag| on the error case and we can save a few lines of code.
Attachment #8913286 - Flags: review?(jdemooij)
Part 13:
- These methods were no longer called.
Attachment #8913287 - Flags: review?(jdemooij)
Part 14:
- JSSubString is only used in builtin/RegExp.cpp, so let's move it into that file.
Attachment #8913288 - Flags: review?(jdemooij)
Part 15:
- js_isidstart, js_isident, and js_isspace are only used for js::unicode methods, so it's a bit cleaner to move the definitions into vm/Unicode.cpp.
- But because vm/Unicode.cpp is a generated file, the easiest way to add the definitions is modifying vm/make_unicode.py to write the lookup tables.
- Also updates vm/make_unicode.py to avoid generating an extra return statement (bug 1381253) and remove an unused #define from jsstr.cpp.
Attachment #8913290 - Flags: review?(jdemooij)
Comment on attachment 8913275 [details] [diff] [review]
bug1403911-part1-jsstr-chartraits.patch

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

::: js/src/jsapi-tests/testUbiNode.cpp
@@ +156,5 @@
>  
>      UniqueTwoByteChars ctorName;
>      CHECK(JS::ubi::Node(&val.toObject()).jsObjectConstructorName(cx, ctorName));
>      CHECK(ctorName);
> +    CHECK(EqualChars(ctorName.get(), u"Ctor", js_strlen(u"Ctor")));

It doesn't matter too much, but this will now pass when ctorName is something like CtorFoo (a prefix but not an exact match), right? Maybe add + 1 to the js_strlen result with a comment that's done to include the null terminator?

Same for the other jsapi-test.
Attachment #8913275 - Flags: review?(jdemooij) → review+
Comment on attachment 8913276 [details] [diff] [review]
bug1403911-part2-enable-memcmp-on-linux.patch

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

Wow, big perf improvement! Using the same code on all platforms is great.
Attachment #8913276 - Flags: review?(jdemooij) → review+
Attachment #8913277 - Flags: review?(jdemooij) → review+
Comment on attachment 8913278 [details] [diff] [review]
bug1403911-part4-avoid-noninlined-isregexp-call.patch

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

::: js/src/jsstr.cpp
@@ +2052,5 @@
>      return true;
>  }
>  
> +static bool
> +ReportErrorIfFirstArgIsRegExp(JSContext* cx, const CallArgs& args)

So compilers do inline this function?
Attachment #8913278 - Flags: review?(jdemooij) → review+
Attachment #8913279 - Flags: review?(jdemooij) → review+
Attachment #8913280 - Flags: review?(jdemooij) → review+
Attachment #8913281 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #18)
> > +static bool
> > +ReportErrorIfFirstArgIsRegExp(JSContext* cx, const CallArgs& args)
> 
> So compilers do inline this function?

Yes, gcc (5.4) and clang (4.0) seem to inline the function (there was no performance difference when I added MOZ_ALWAYS_INLINE). But for MSVC (2015) we seem to need MOZ_ALWAYS_INLINE: Without MOZ_ALWAYS_INLINE it only improved from 200ms -> 190ms, with MOZ_ALWAYS_INLINE it improved from 200ms -> 180ms.
Attachment #8913282 - Flags: review?(jdemooij) → review+
Comment on attachment 8913283 [details] [diff] [review]
bug1403911-part9-remove-inflatestring-out-param.patch

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

::: js/src/jsstr.cpp
@@ +3879,5 @@
>  js_strchr_limit(const Latin1Char* s, char16_t c, const Latin1Char* limit);
>  
>  template const char16_t*
>  js_strchr_limit(const char16_t* s, char16_t c, const char16_t* limit);
> + 

Nit: trailing whitespace
Attachment #8913283 - Flags: review?(jdemooij) → review+
Attachment #8913284 - Flags: review?(jdemooij) → review+
Attachment #8913285 - Flags: review?(jdemooij) → review+
Attachment #8913286 - Flags: review?(jdemooij) → review+
Attachment #8913287 - Flags: review?(jdemooij) → review+
Attachment #8913288 - Flags: review?(jdemooij) → review+
Attachment #8913290 - Flags: review?(jdemooij) → review+
Thank you! Great patches and performance improvements \o/
Update part 1 per review comments, carrying r+.
Attachment #8913275 - Attachment is obsolete: true
Attachment #8913760 - Flags: review+
Update part 4 to add MOZ_ALWAYS_INLINE to force inlining the helper function (needed for MSVC, comment #19). Carrying r+.
Attachment #8913278 - Attachment is obsolete: true
Attachment #8913761 - Flags: review+
Update part 8 to apply cleanly on inbound, no change in behaviour, carrying r+.
Attachment #8913282 - Attachment is obsolete: true
Attachment #8913763 - Flags: review+
Update part 9 per review comments, carrying r+.
Attachment #8913283 - Attachment is obsolete: true
Attachment #8913764 - Flags: review+
Part 16 (The last patch for this bug!):

Changes in Encode():
- Change some variables from char16_t to Latin1Char, to ensure |sb.append(...)| can skip the "Is this char16_t a Latin-1 character?" checks.
- Inlined Latin-1 to UFT-8 encoding (this change improved the µ-benchmark below noticeably under gcc/clang).

Changes in js::EncodeURI():
- Without the reinterpret cast the µ-benchmark only improved to 360ms under gcc. I have no idea why this change is necessary for gcc...!

Both changes improved this µ-benchmark from 400ms to 300ms for me (gcc/clang):

    // "+" and "/" are both percent-encoded in encodeURIComponent.
    var s = ["+".repeat(10), "/".repeat(10)];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        q += encodeURIComponent(s[i & 1]).length;
    }
    print(dateNow() - t);


For consistency I've also changed Decode() to call Latin-1 StringBuffer methods. It looks like this could have improved some µ-benchmarks by 2-3%, but the difference could also be noise. And I've moved the |reservedSet| check into the ASCII-case (|if (!(B & 0x80))|), because ASCII percent-encoded characters are not allowed (this is tested in the |if (n == 1 || n > 4)| if-statement).
Attachment #8913774 - Flags: review?(jdemooij)
(In reply to André Bargull [:anba] from comment #26)
> Changes in js::EncodeURI():
> - Without the reinterpret cast the µ-benchmark only improved to 360ms under
> gcc. I have no idea why this change is necessary for gcc...!

Because it seems to alter gcc's inlining decisions. When I added MOZ_ALWAYS_INLINE to Encode(JSContext*, HandleLinearString, const bool*, const bool*, MutableHandleValue), I got the same results as with the reinterpret_cast change...
Comment on attachment 8913774 [details] [diff] [review]
bug1403911-part16-latin1-encode-decode-uri.patch

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

Nice.

::: js/src/jsstr.cpp
@@ +4031,1 @@
>          if (c < 128 && (unescapedSet[c] || (unescapedSet2 && unescapedSet2[c]))) {

Do you think it's worth removing the unescapedSet argument, if it's always js_isUriUnescaped? And maybe we could use templates to switch between js_isUriUnescaped or js_isUriReservedPlusPound, so we always have to do a single lookup without a null check? It's possible I'm missing something.

@@ +4125,5 @@
>  
>              uint32_t B = JS7_UNHEX(chars[k+1]) * 16 + JS7_UNHEX(chars[k+2]);
>              k += 2;
>              if (!(B & 0x80)) {
> +                MOZ_ASSERT(B < 128);

Could/should we change this to just |if (B < 128) {| ?

@@ +4268,5 @@
>  
>  bool
>  js::EncodeURI(JSContext* cx, StringBuffer& sb, const char* chars, size_t length)
>  {
> +    EncodeResult result = Encode(sb, reinterpret_cast<const Latin1Char*>(chars), length,

Adding the MOZ_ALWAYS_INLINE might be more robust :)
Attachment #8913774 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #28)
> > +    EncodeResult result = Encode(sb, reinterpret_cast<const Latin1Char*>(chars), length,
> 
> Adding the MOZ_ALWAYS_INLINE might be more robust :)

Actually we probably should do both? The reinterpret_cast is nice to have.
(In reply to Jan de Mooij [:jandem] from comment #28)
> ::: js/src/jsstr.cpp
> @@ +4031,1 @@
> >          if (c < 128 && (unescapedSet[c] || (unescapedSet2 && unescapedSet2[c]))) {
> 
> Do you think it's worth removing the unescapedSet argument, if it's always
> js_isUriUnescaped? And maybe we could use templates to switch between
> js_isUriUnescaped or js_isUriReservedPlusPound, so we always have to do a
> single lookup without a null check? It's possible I'm missing something.

I've removed the argument for |js_isUriUnescaped|, but kept the argument for the optional |js_isUriReservedPlusPound|, because replacing it with template parameters didn't give a noticeable performance improvement. Interestingly using template parameters did again lead to different inlining decisions in GCC, this time I had to annotate the other Encode function with MOZ_NEVER_INLINE to avoid the performance difference noted in comment #26. So I guess it's best to keep MOZ_ALWAYS_INLINE resp. MOZ_NEVER_INLINE for the Encode functions. :-)


> @@ +4125,5 @@
> >  
> >              uint32_t B = JS7_UNHEX(chars[k+1]) * 16 + JS7_UNHEX(chars[k+2]);
> >              k += 2;
> >              if (!(B & 0x80)) {
> > +                MOZ_ASSERT(B < 128);
> 
> Could/should we change this to just |if (B < 128) {| ?

Changed.

> @@ +4268,5 @@
> >  
> >  bool
> >  js::EncodeURI(JSContext* cx, StringBuffer& sb, const char* chars, size_t length)
> >  {
> > +    EncodeResult result = Encode(sb, reinterpret_cast<const Latin1Char*>(chars), length,
> 
> Adding the MOZ_ALWAYS_INLINE might be more robust :)

Will do.
Update part 7 to apply cleanly on inbound, no change in behaviour, carrying r+.
Attachment #8913281 - Attachment is obsolete: true
Attachment #8914376 - Flags: review+
Updated part 16 per review comments, carrying r+.
Attachment #8913774 - Attachment is obsolete: true
Attachment #8914378 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/878775315411
Part 1: Replace hand-written implementation for js_strlen with char_traits. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/d34adb57e332
Part 2: Enable memcmp for string matching on Linux. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/870fe2993476
Part 3: Remove unnecessary rooting in String.prototype.trim, .normalize functions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/59e81b71acea
Part 4: Don't call js::IsRegExp in the fast-paths for string methods. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b00d68c8ef9
Part 5: Don't overwrite call arguments when converting arguments in string methods. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/763b978156fa
Part 6: Don't atomize the pattern in str_replace_string_raw. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/73df32fe6776
Part 7: Fix style nits in jsstr.cpp. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e829cb93e4
Part 8: Skip extra rooting and concat-operation in BuildDollarReplacement. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe59ecab8f56
Part 9: Remove unused out-param from InflateString. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c077e3738f
Part 10: Use MaybeHasInterestingSymbolProperty to speed-up toPrimitive check in HasNoToPrimitiveMethodPure. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3d65c1e10e
Part 11: Call internal self-hosting function through CallSelfHostedFunction to reduce code duplication. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/52a7b6a2aa89
Part 12: Inline RegExp flag validation and correct return type. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/eff4be681f2b
Part 13: Remove unused methods from RegExpStatics class. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc39e017590
Part 14: Move JSSubString struct to its remaining callers. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/9da8518c5abe
Part 15: Generate ASCII lookup tables for Unicode methods. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6282a67845
Part 16: Special case Latin-1 strings in encodeURI/decodeURI. r=jandem
Keywords: checkin-needed
Duplicate of this bug: 1019114
You need to log in before you can comment on or make changes to this bug.