Use std::to_chars for int-to-string conversion
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
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 | |
Bug 1934561 - Part 13: Remove base 10 restriction from BigInt::toStringSingleDigitBaseTen. r=jandem!
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.
Assignee | ||
Comment 1•2 months ago
|
||
Local tests show no significant performance changes for base 10 and base 16,
but improved performance for base 2 and base 8.
Assignee | ||
Comment 2•2 months ago
|
||
Instead of always returning the start of the buffer, return the number of
written digits.
Assignee | ||
Comment 3•2 months ago
|
||
This removes the last caller to BackfillIndexInCharBuffer
.
Assignee | ||
Comment 4•2 months ago
|
||
At least debug-assert that ToShortest
was successful.
Assignee | ||
Comment 5•2 months ago
|
||
IndexToString
is only called through KeyStringifier
from SerializeJSONArray
,
which makes it unlikely that the cache is ever used.
Assignee | ||
Comment 6•2 months ago
|
||
The compiler optimises away this comparison anyway.
Assignee | ||
Comment 7•2 months ago
|
||
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.
Assignee | ||
Comment 8•2 months ago
|
||
This avoids unnecessary int32 -> double -> int32 conversions when called from
js::Int32ToStringWithBase
.
Assignee | ||
Comment 9•2 months ago
|
||
The nullptr
checks in LookupDtoaCache
and CacheNumber
are unnecessary
and after removing them, both functions are trivial to inline into their
callers.
Assignee | ||
Comment 10•2 months ago
|
||
And then remove the no longer needed null-termination.
Assignee | ||
Comment 11•2 months ago
|
||
This avoids another unnecessary branch to lookup static strings, because static
strings are already handled earlier.
Assignee | ||
Comment 12•2 months ago
|
||
Change Int32ToStringWithBase
to return JSLinearString
and add AllowGC
template parameter in preparation for the next part.
Assignee | ||
Comment 13•2 months ago
|
||
Allow any base in BigInt::toStringSingleDigitBaseTen
.
Assignee | ||
Comment 14•2 months ago
|
||
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.
Assignee | ||
Comment 15•2 months ago
|
||
charsRequired
can never exceed JSString::MAX_LENGTH
. Assert this statically
and at runtime.
Assignee | ||
Comment 16•2 months ago
|
||
This avoids unnecessary rooting in js::Int32ToStringWithBase
.
Updated•2 months ago
|
Comment 17•2 months ago
|
||
Comment 18•2 months ago
|
||
Backed out for causing build bustages
Backout link: https://hg.mozilla.org/integration/autoland/rev/af479be432c92262813deb31a266b76e94c4f412
Assignee | ||
Comment 19•2 months ago
|
||
Looks like old Clang versions (<12) don't pick the correct function overload: https://godbolt.org/z/bWdbMoz67
Comment 20•2 months ago
|
||
Comment 21•2 months ago
|
||
bugherder |
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
Description
•