1934561 - Use std::to_chars for int-to-string conversion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[André Bargull [:anba]]

Use std::to_chars for int-to-string conversion, because it's not slower than our own implementation and also provides optimised code paths for base 2 (bug 1932864) and base 8. Also perform additional optimisations to further improve the performance for int-to-string conversions.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 1: Use std::to_chars for int-to-string conversion. r=jandem!

Local tests show no significant performance changes for base 10 and base 16,
but improved performance for base 2 and base 8.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 2: Return length from int-to-string conversion functions. r=jandem!

Instead of always returning the start of the buffer, return the number of
written digits.

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 3: Use std::to_chars for IndexToIdSlow. r=jandem!

This removes the last caller to BackfillIndexInCharBuffer.

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 4: Don't ignore result from DoubleToStringConverter::ToShortest. r=jandem!

At least debug-assert that ToShortest was successful.

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 5: Remove dtoaCache lookup from IndexToString. r=jandem!

IndexToString is only called through KeyStringifier from SerializeJSONArray,
which makes it unlikely that the cache is ever used.

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 6: Remove an unnecessary comparison. r=jandem!

The compiler optimises away this comparison anyway.

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 7: Use faster NewInlineString call in InlineCharBuffer. r=jandem!

NewInlineString with a fixed-length array is a bit faster than the version which
takes a mozilla::Range, because the former can use std::memcpy with a constant
number of bytes to copy.

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 8: Move int32 code path out of NumberToStringWithBase. r=jandem!

This avoids unnecessary int32 -> double -> int32 conversions when called from
js::Int32ToStringWithBase.

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 9: Inline LookupDtoaCache and CacheNumber. r=jandem!

The nullptr checks in LookupDtoaCache and CacheNumber are unnecessary
and after removing them, both functions are trivial to inline into their
callers.

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 10: Inline Int32ToCStringWithBase. r=jandem!

And then remove the no longer needed null-termination.

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 11: Skip static strings check in Int32ToStringWithBase. r=jandem!

This avoids another unnecessary branch to lookup static strings, because static
strings are already handled earlier.

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 12: Return linear string from Int32ToStringWithBase. r=jandem!

Change Int32ToStringWithBase to return JSLinearString and add AllowGC
template parameter in preparation for the next part.

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 13: Remove base 10 restriction from BigInt::toStringSingleDigitBaseTen. r=jandem!

Allow any base in BigInt::toStringSingleDigitBaseTen.

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 14: Avoid duplicate character allocation. r=jandem!

InlineCharBuffer doesn't support nursery string characters allocation, which
makes it slower than using a short-lived malloc allocation.

Replacing InlineCharBuffer with StringChars for other code will happen in
bug 1910084.

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 15: Change string overflow check to assertion. r=jandem!

charsRequired can never exceed JSString::MAX_LENGTH. Assert this statically
and at runtime.

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1934561 - Part 16: Remove unnecessary rooting for StringTo{Lower,Upper}Case. r=jandem!

This avoids unnecessary rooting in js::Int32ToStringWithBase.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82c04d36c600
Part 1: Use std::to_chars for int-to-string conversion. r=jandem
https://hg.mozilla.org/integration/autoland/rev/44eeb8851ff9
Part 2: Return length from int-to-string conversion functions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/4012ff039521
Part 3: Use std::to_chars for IndexToIdSlow. r=jandem
https://hg.mozilla.org/integration/autoland/rev/724447ba8daa
Part 4: Don't ignore result from DoubleToStringConverter::ToShortest. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a3ecc57f4625
Part 5: Remove dtoaCache lookup from IndexToString. r=jandem
https://hg.mozilla.org/integration/autoland/rev/07228a958e82
Part 6: Remove an unnecessary comparison. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9d9186e94505
Part 7: Use faster NewInlineString call in InlineCharBuffer. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b9b07943d8a3
Part 8: Move int32 code path out of NumberToStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/5f83b3480245
Part 9: Inline LookupDtoaCache and CacheNumber. r=jandem
https://hg.mozilla.org/integration/autoland/rev/06160277b07b
Part 10: Inline Int32ToCStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/404f6e3ecaab
Part 11: Skip static strings check in Int32ToStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d160292ae757
Part 12: Return linear string from Int32ToStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9a2ce63ca64b
Part 13: Remove base 10 restriction from BigInt::toStringSingleDigitBaseTen. r=jandem
https://hg.mozilla.org/integration/autoland/rev/967331695aeb
Part 14: Avoid duplicate character allocation. r=jandem
https://hg.mozilla.org/integration/autoland/rev/493cba8de581
Part 15: Change string overflow check to assertion. r=jandem
https://hg.mozilla.org/integration/autoland/rev/e2796e58aabc
Part 16: Remove unnecessary rooting for StringTo{Lower,Upper}Case. r=jandem

Comment 18

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages

Backout link: https://hg.mozilla.org/integration/autoland/rev/af479be432c92262813deb31a266b76e94c4f412

Push with failures

Failure log -> ERROR - /builds/worker/checkouts/gecko/js/src/vm/StringType-inl.h:772:42: error: non-constant-expression cannot be narrowed from type 'char16_t' to 'char' in initializer list

Comment 19

[André Bargull [:anba]]

Looks like old Clang versions (<12) don't pick the correct function overload: https://godbolt.org/z/bWdbMoz67

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74c5b5b0da08
Part 1: Use std::to_chars for int-to-string conversion. r=jandem
https://hg.mozilla.org/integration/autoland/rev/359f61f25b5b
Part 2: Return length from int-to-string conversion functions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a6345fe9bdf9
Part 3: Use std::to_chars for IndexToIdSlow. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c31d38a9a114
Part 4: Don't ignore result from DoubleToStringConverter::ToShortest. r=jandem
https://hg.mozilla.org/integration/autoland/rev/deef27c42be9
Part 5: Remove dtoaCache lookup from IndexToString. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d041ef382a36
Part 6: Remove an unnecessary comparison. r=jandem
https://hg.mozilla.org/integration/autoland/rev/47a94f9ad0b8
Part 7: Use faster NewInlineString call in InlineCharBuffer. r=jandem
https://hg.mozilla.org/integration/autoland/rev/0139963735f2
Part 8: Move int32 code path out of NumberToStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/500fef7a42c7
Part 9: Inline LookupDtoaCache and CacheNumber. r=jandem
https://hg.mozilla.org/integration/autoland/rev/f8af1afa3c76
Part 10: Inline Int32ToCStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a5215e9cfadf
Part 11: Skip static strings check in Int32ToStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c2b285378c00
Part 12: Return linear string from Int32ToStringWithBase. r=jandem
https://hg.mozilla.org/integration/autoland/rev/5c2eb0b3c1fe
Part 13: Remove base 10 restriction from BigInt::toStringSingleDigitBaseTen. r=jandem
https://hg.mozilla.org/integration/autoland/rev/fcc2e57eca34
Part 14: Avoid duplicate character allocation. r=jandem
https://hg.mozilla.org/integration/autoland/rev/20001abc3177
Part 15: Change string overflow check to assertion. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ee58b18cd342
Part 16: Remove unnecessary rooting for StringTo{Lower,Upper}Case. r=jandem

Comment 21

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/74c5b5b0da08
https://hg.mozilla.org/mozilla-central/rev/359f61f25b5b
https://hg.mozilla.org/mozilla-central/rev/a6345fe9bdf9
https://hg.mozilla.org/mozilla-central/rev/c31d38a9a114
https://hg.mozilla.org/mozilla-central/rev/deef27c42be9
https://hg.mozilla.org/mozilla-central/rev/d041ef382a36
https://hg.mozilla.org/mozilla-central/rev/47a94f9ad0b8
https://hg.mozilla.org/mozilla-central/rev/0139963735f2
https://hg.mozilla.org/mozilla-central/rev/500fef7a42c7
https://hg.mozilla.org/mozilla-central/rev/f8af1afa3c76
https://hg.mozilla.org/mozilla-central/rev/a5215e9cfadf
https://hg.mozilla.org/mozilla-central/rev/c2b285378c00
https://hg.mozilla.org/mozilla-central/rev/5c2eb0b3c1fe
https://hg.mozilla.org/mozilla-central/rev/fcc2e57eca34
https://hg.mozilla.org/mozilla-central/rev/20001abc3177
https://hg.mozilla.org/mozilla-central/rev/ee58b18cd342