Skip to content

Conversation

@xwu
Copy link
Collaborator

@xwu xwu commented Nov 9, 2025

A follow-up to #85180.

First, make the guarantee that our Swift-native implementation of integer-to-ASCII conversion always fills the suffix of the given mutable span.

(Some minor swift-format corrections interspersed.)

Then, use that guarantee to work in 64-bit (or rather, for most bases, 56-bit) chunks for larger integers.

@xwu xwu requested a review from a team as a code owner November 9, 2025 23:08
@xwu

This comment was marked as outdated.

@xwu

This comment was marked as outdated.

@xwu

This comment was marked as outdated.

@xwu xwu marked this pull request as draft November 10, 2025 03:44
@xwu

This comment was marked as outdated.

@xwu

This comment was marked as outdated.

@xwu
Copy link
Collaborator Author

xwu commented Nov 16, 2025

@swift-ci please benchmark

@xwu

This comment was marked as outdated.

@xwu

This comment has been minimized.

@xwu
Copy link
Collaborator Author

xwu commented Nov 16, 2025

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
CxxStringConversion.cxx.to.swift 108.0 150.5 +39.4% 0.72x (?)
String.replaceSubrange.Substring 10.341 11.36 +9.9% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
IntegerToString.UInt128.hex 197.2 32.27 -83.6% 6.11x
IntegerToString.UInt128.decimal 156.0 37.9 -75.7% 4.12x
Data.hash.Medium 38.286 30.767 -19.6% 1.24x

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
ArrayAppendGenericStructs 1615.0 1770.0 +9.6% 0.91x (?)
DataToStringSmall 144.737 158.333 +9.4% 0.91x (?)
String.replaceSubrange.String 9.359 10.133 +8.3% 0.92x (?)
String.replaceSubrange.Substring 10.019 10.781 +7.6% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
IntegerToString.UInt128.hex 192.083 31.533 -83.6% 6.09x
IntegerToString.UInt128.decimal 152.5 37.778 -75.2% 4.04x
Data.hash.Medium 38.103 30.085 -21.0% 1.27x (?)
MapReduceAnyCollection 174.167 150.625 -13.5% 1.16x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
Data.hash.Small 234.125 252.375 +7.8% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
IntegerToString.UInt128.hex 204.111 42.759 -79.1% 4.77x
IntegerToString.UInt128.decimal 163.75 48.938 -70.1% 3.35x
Data.hash.Medium 41.636 34.136 -18.0% 1.22x (?)
ArrayAppendGenericStructs 1377.5 1137.5 -17.4% 1.21x (?)
StringAdder 414.0 371.4 -10.3% 1.11x (?)
CharacterLiteralsSmall 732.667 660.0 -9.9% 1.11x (?)
Array.removeAll.keepingCapacity.Object 9.348 8.538 -8.7% 1.09x (?)
StringInterpolationSmall 1990.0 1834.0 -7.8% 1.09x (?)

Code size: -swiftlibs

How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 32 GB

@xwu

This comment was marked as outdated.

@xwu

This comment was marked as outdated.


var span = unsafe buffer.mutableSpan
let textRange = unsafe _BinaryIntegerToASCII(
// Work in chunks for speed.
Copy link
Collaborator Author

@xwu xwu Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the beginning of the substantive part of this PR. As before, it's been tested thoroughly in a separate package.

I've tried to be very careful of the following, which is not so testable (at least with my setup), but it could use some eyes during review:

  • Any endianness issues with going chunk-by-chunk in the specific way that I do (I don't think so, but I've been fooled in the past).
  • Whether the strategy described above (see line R1695) with safetyMargin is indeed capable of avoiding buffer overflows even with hypothetical end-user BinaryInteger types that may be unusually sized or even incorrectly implemented.

@xwu xwu marked this pull request as ready for review November 16, 2025 21:02
@xwu
Copy link
Collaborator Author

xwu commented Nov 16, 2025

@swift-ci smoke test

@xwu
Copy link
Collaborator Author

xwu commented Nov 17, 2025

@swift-ci smoke test macOS platform

@xwu
Copy link
Collaborator Author

xwu commented Nov 17, 2025

@swift-ci smoke test Windows platform

@xwu
Copy link
Collaborator Author

xwu commented Nov 17, 2025

@shahmishal It seems requesting a smoke test for any platform is now triggering a smoke test for macOS platform too--is that intentional?

@xwu
Copy link
Collaborator Author

xwu commented Nov 18, 2025

@swift-ci please test Windows platform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant