Closed Bug 1934561 Opened 2 months ago Closed 2 months ago

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

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
135 Branch
Tracking Status
firefox135 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(16 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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.

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

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

This removes the last caller to BackfillIndexInCharBuffer.

At least debug-assert that ToShortest was successful.

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

The compiler optimises away this comparison anyway.

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.

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

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

And then remove the no longer needed null-termination.

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

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

Allow any base in BigInt::toStringSingleDigitBaseTen.

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.

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

This avoids unnecessary rooting in js::Int32ToStringWithBase.

Severity: -- → N/A
Priority: -- → P3
Pushed by andre.bargull@gmail.com: 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

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

Flags: needinfo?(andrebargull)
Pushed by andre.bargull@gmail.com: 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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: