Fix potentially undefined behavior in StringObject.nativeStorage and document Builtin.unsafeBitCast #63631

atrick · 2023-02-13T20:21:59Z

Speculatively fixing this to rule out potential miscompiles.

The compiler needs to know if a reference is being materialized out of thin air. The proper way to do that is with the Unmanaged API.

Under the hood, this forces the reference into an "unowned(unsafe)" variable which the reference must be reloaded from. That tells the compiler that it can't optimize some seemingly unrelated object which the reference may happen to refer to at runtime.

/// Warning: Casting from an integer or a pointer type to a reference type
/// is undefined behavior. It may result in incorrect code in any future
/// compiler release. To convert a bit pattern to a reference type:
/// 1. convert the bit pattern to an UnsafeRawPointer.
/// 2. create an unmanaged reference using Unmanaged.fromOpaque()
/// 3. obtain a managed reference using Unmanaged.takeUnretainedValue()
/// The programmer must ensure that the resulting reference has already been
/// manually retained.

Speculatively fixing this to rule out potential miscompiles. The compiler needs to know if a reference is being materialized out of thin air. The proper way to do that is with the Unmanaged API. Under the hood, this forces the reference into an "unowned(unsafe)" variable which the reference must be reloaded from. That tells the compiler that it can't optimize some seemingly unrelated object which the reference may happen to refer to at runtime. /// Warning: Casting from an integer or a pointer type to a reference type /// is undefined behavior. It may result in incorrect code in any future /// compiler release. To convert a bit pattern to a reference type: /// 1. convert the bit pattern to an UnsafeRawPointer. /// 2. create an unmanaged reference using Unmanaged.fromOpaque() /// 3. obtain a managed reference using Unmanaged.takeUnretainedValue() /// The programmer must ensure that the resulting reference has already been /// manually retained.

atrick · 2023-02-13T20:22:11Z

@swift-ci test

atrick · 2023-02-13T20:22:22Z

@swift-ci benchmark

lorentey · 2023-02-13T20:56:24Z

This is increasing my anxiety about some unsafeBitCasts I introduced in #62717: 🤔

extension StringProtocol {
  public // SPI(Foundation)
  func _toUTF16Offsets(_ indices: Range<Index>) -> Range<Int> {
    if Self.self == String.self {
      let s = unsafeBitCast(self, to: String.self)
      return s.utf16._offsetRange(for: indices, from: s.startIndex)
    }
    if Self.self == Substring.self {
      let s = unsafeBitCast(self, to: Substring.self)
      return s._slice._base.utf16._offsetRange(for: indices, from: s.startIndex)
    }
    ...
  }
}

glessard

Thanks for the forceful unsafeBitCast clarification.

glessard · 2023-02-13T22:30:23Z

Benchmark results:

Performance (x86_64): -O

Regression	OLD	NEW	DELTA	RATIO
StringWithCString2	0.0	0.002	+200.0%	0.33x (?)
StringWordBuilderReservingCapacity	1737.273	3041.429	+75.1%	0.57x
StringWordBuilder	1762.727	3068.333	+74.1%	0.57x
Join	120.769	199.222	+65.0%	0.61x (?)
RemoveWhereFilterString	238.1	349.714	+46.9%	0.68x
AngryPhonebook.Strasse	644.5	888.5	+37.9%	0.73x
AngryPhonebook.Armenian	661.333	909.0	+37.4%	0.73x
AngryPhonebook.Cyrillic	693.667	930.0	+34.1%	0.75x (?)
String.replaceSubrange.String	16.457	21.882	+33.0%	0.75x (?)
String.replaceSubrange.Substring	18.281	24.276	+32.8%	0.75x
InsertCharacterEndIndexNonASCII	40.5	51.92	+28.2%	0.78x (?)
InsertCharacterEndIndex	124.692	157.833	+26.6%	0.79x (?)
LineSink.characters.complex	403.167	510.25	+26.6%	0.79x
EqualSubstringSubstringGenericEquatable	24.531	30.92	+26.0%	0.79x (?)
InsertCharacterTowardsEndIndex	134.769	168.455	+25.0%	0.80x
LessSubstringSubstringGenericComparable	24.647	30.8	+25.0%	0.80x
EqualSubstringString	25.059	31.16	+24.3%	0.80x (?)
Breadcrumbs.MutatedUTF16ToIdx.ASCII	2.966	3.679	+24.0%	0.81x (?)
EqualSubstringSubstring	25.192	31.24	+24.0%	0.81x (?)
EqualStringSubstring	25.357	31.346	+23.6%	0.81x (?)
Breadcrumbs.MutatedIdxToUTF16.ASCII	2.976	3.675	+23.5%	0.81x (?)
LessSubstringSubstring	25.941	30.813	+18.8%	0.84x
FloatingPointPrinting_Float_interpolated	26738.462	31420.0	+17.5%	0.85x (?)
StringComparison_longSharedPrefix	211.0	240.8	+14.1%	0.88x (?)
InsertCharacterTowardsEndIndexNonASCII	91.25	102.813	+12.7%	0.89x (?)
Breadcrumbs.MutatedIdxToUTF16.Mixed	189.917	213.444	+12.4%	0.89x (?)
InsertCharacterStartIndex	297.391	333.095	+12.0%	0.89x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed	186.833	208.8	+11.8%	0.89x (?)
InsertCharacterStartIndexNonASCII	171.636	191.429	+11.5%	0.90x (?)
HashTest	484.444	531.25	+9.7%	0.91x (?)

Improvement	OLD	NEW	DELTA	RATIO
FlattenListLoop	1616.0	1386.0	-14.2%	1.17x (?)
NormalizedIterator_slowerPrenormal	317.25	275.556	-13.1%	1.15x (?)
NormalizedIterator_fastPrenormal	521.277	460.926	-11.6%	1.13x (?)
StringHasSuffixAscii	1524.545	1372.143	-10.0%	1.11x (?)
NormalizedIterator_nonBMPSlowestPrenormal	453.455	410.435	-9.5%	1.10x (?)
Data.hash.Medium	26.769	24.391	-8.9%	1.10x (?)
NormalizedIterator_emoji	359.84	328.16	-8.8%	1.10x
StringComparison_ascii	201.455	186.692	-7.3%	1.08x (?)
Calculator	151.583	141.2	-6.8%	1.07x (?)

Code size: -O

Performance (x86_64): -Osize

Regression	OLD	NEW	DELTA	RATIO
StringWordBuilderReservingCapacity	1738.182	3042.857	+75.1%	0.57x
StringWordBuilder	1763.0	3068.571	+74.1%	0.57x
Join	120.929	199.0	+64.6%	0.61x
RemoveWhereFilterString	237.2	349.167	+47.2%	0.68x
AngryPhonebook.Strasse	644.667	888.5	+37.8%	0.73x
AngryPhonebook.Armenian	661.667	909.5	+37.5%	0.73x
StringInterpolationManySmallSegments	7453.333	10075.0	+35.2%	0.74x (?)
AngryPhonebook.Cyrillic	694.0	930.0	+34.0%	0.75x (?)
String.replaceSubrange.Substring	18.294	24.405	+33.4%	0.75x (?)
String.replaceSubrange.String	16.543	21.884	+32.3%	0.76x
InsertCharacterEndIndexNonASCII	40.529	51.96	+28.2%	0.78x (?)
LineSink.characters.complex	402.0	511.5	+27.2%	0.79x (?)
InsertCharacterEndIndex	124.133	157.667	+27.0%	0.79x (?)
EqualStringSubstring	24.571	30.808	+25.4%	0.80x
EqualSubstringString	24.648	30.813	+25.0%	0.80x (?)
EqualSubstringSubstringGenericEquatable	24.679	30.808	+24.8%	0.80x (?)
Breadcrumbs.MutatedUTF16ToIdx.ASCII	2.964	3.683	+24.2%	0.80x (?)
LessSubstringSubstringGenericComparable	25.059	30.917	+23.4%	0.81x (?)
EqualSubstringSubstring	25.0	30.8	+23.2%	0.81x
InsertCharacterTowardsEndIndex	143.667	176.615	+22.9%	0.81x (?)
Breadcrumbs.MutatedIdxToUTF16.ASCII	3.008	3.673	+22.1%	0.82x
StringInterpolation	6090.0	7311.765	+20.1%	0.83x (?)
LessSubstringSubstring	25.818	30.925	+19.8%	0.83x (?)
FloatingPointPrinting_Float_interpolated	26588.235	31240.0	+17.5%	0.85x (?)
StringComparison_longSharedPrefix	208.1	241.3	+16.0%	0.86x (?)
FloatingPointPrinting_Double_interpolated	27733.333	32109.091	+15.8%	0.86x (?)
FloatingPointPrinting_Float80_interpolated	33327.273	38133.333	+14.4%	0.87x (?)
RemoveWhereSwapInts	13.685	15.588	+13.9%	0.88x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed	185.182	208.2	+12.4%	0.89x (?)
Breadcrumbs.MutatedIdxToUTF16.Mixed	190.0	212.9	+12.1%	0.89x (?)
InsertCharacterStartIndex	298.913	333.81	+11.7%	0.90x (?)
InsertCharacterStartIndexNonASCII	172.727	191.25	+10.7%	0.90x (?)
HashTest	500.625	546.667	+9.2%	0.92x (?)
SetUnionInt25	71.37	77.08	+8.0%	0.93x (?)
NSStringConversion.InlineBuffer.UTF8	703.0	757.5	+7.8%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
RemoveWhereMoveInts	13.622	9.807	-28.0%	1.39x
StrComplexWalk	3125.0	2777.143	-11.1%	1.13x (?)
Data.hash.Medium	26.769	24.379	-8.9%	1.10x (?)
UTF8Decode_InitFromBytes	141.1	130.0	-7.9%	1.09x (?)
NormalizedIterator_emoji	347.692	320.966	-7.7%	1.08x (?)
StringHasSuffixAscii	1481.0	1370.833	-7.4%	1.08x (?)
NormalizedIterator_slowerPrenormal	297.674	276.818	-7.0%	1.08x (?)
NormalizedIterator_nonBMPSlowestPrenormal	437.059	407.0	-6.9%	1.07x (?)
UTF8Decode_InitFromData	135.0	125.833	-6.8%	1.07x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression	OLD	NEW	DELTA	RATIO
StringWordBuilderReservingCapacity	1805.556	3090.0	+71.1%	0.58x
StringWordBuilder	1831.25	3118.333	+70.3%	0.59x
Join	128.0	205.625	+60.6%	0.62x
StringInterpolationManySmallSegments	9200.0	13416.667	+45.8%	0.69x (?)
AngryPhonebook.Strasse	644.667	888.5	+37.8%	0.73x (?)
AngryPhonebook.Armenian	661.333	910.0	+37.6%	0.73x (?)
AngryPhonebook.Cyrillic	694.333	929.5	+33.9%	0.75x (?)
String.replaceSubrange.Substring	19.176	25.259	+31.7%	0.76x (?)
RemoveWhereFilterString	333.857	438.0	+31.2%	0.76x (?)
String.replaceSubrange.String	17.417	22.75	+30.6%	0.77x (?)
LineSink.characters.complex	495.5	618.0	+24.7%	0.80x
EqualSubstringSubstringGenericEquatable	27.654	33.967	+22.8%	0.81x (?)
InsertCharacterEndIndex	172.833	210.222	+21.6%	0.82x
LessSubstringSubstring	29.433	35.519	+20.7%	0.83x (?)
InsertCharacterEndIndexNonASCII	57.304	69.059	+20.5%	0.83x (?)
InsertCharacterTowardsEndIndex	157.9	188.0	+19.1%	0.84x (?)
FloatingPointPrinting_Double_interpolated	40975.0	47850.0	+16.8%	0.86x (?)
StringInterpolation	6733.333	7858.824	+16.7%	0.86x (?)
LessSubstringSubstringGenericComparable	29.226	33.931	+16.1%	0.86x (?)
FloatingPointPrinting_Float_interpolated	38733.333	44800.0	+15.7%	0.86x (?)
EqualStringSubstring	31.208	35.946	+15.2%	0.87x (?)
EqualSubstringString	31.677	36.448	+15.1%	0.87x (?)
EqualSubstringSubstring	30.92	35.357	+14.3%	0.87x (?)
FloatingPointPrinting_Float80_interpolated	53742.857	60771.429	+13.1%	0.88x (?)
CharacterLiteralsLarge	403.6	455.25	+12.8%	0.89x (?)
InsertCharacterStartIndex	351.429	394.737	+12.3%	0.89x (?)
InsertCharacterStartIndexNonASCII	199.222	222.0	+11.4%	0.90x (?)
String.replaceSubrange.ArrChar	49.045	54.227	+10.6%	0.90x (?)
Breadcrumbs.MutatedIdxToUTF16.Mixed	205.091	225.6	+10.0%	0.91x (?)
Breadcrumbs.MutatedUTF16ToIdx.Mixed	201.083	220.6	+9.7%	0.91x (?)
InsertCharacterTowardsEndIndexNonASCII	101.125	109.2	+8.0%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
SubstringRemoveFirst1	0.143	0.0	-99.3%	144.00x (?)
ArrayAppendLatin1Substring	26088.0	23172.0	-11.2%	1.13x (?)
ArrayAppendUTF16Substring	25704.0	22932.0	-10.8%	1.12x (?)
ArrayAppendAsciiSubstring	25722.0	23004.0	-10.6%	1.12x (?)
Data.hash.Medium	31.765	29.381	-7.5%	1.08x (?)

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

atrick · 2023-02-14T02:51:34Z

Don't merge this. We need to figure out how to recover performance first.

lorentey · 2023-02-14T03:04:09Z

Huh, did the old code somehow manage to avoid retaining the storage object? 🤯

If that's what causing the perf regressions, one unimaginative way to fix that would be to have var nativeStorage return an Unmanaged value and to call _withUnsafeGuaranteedRef on it. (Or to replace this property with a withNativeStorage(_:) function that does the same.)

atrick · 2023-02-17T18:32:33Z

Fixed in #63646

atrick requested review from lorentey and nate-chandler February 13, 2023 20:22

lorentey approved these changes Feb 13, 2023

View reviewed changes

glessard approved these changes Feb 13, 2023

View reviewed changes

atrick marked this pull request as draft February 14, 2023 02:54

lorentey mentioned this pull request Feb 14, 2023

[stdlib] Fix potentially undefined behavior in StringObject.nativeStorage and document Builtin.unsafeBitCast #63646

Merged

atrick closed this Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix potentially undefined behavior in StringObject.nativeStorage and document Builtin.unsafeBitCast #63631

Fix potentially undefined behavior in StringObject.nativeStorage and document Builtin.unsafeBitCast #63631

Uh oh!

atrick commented Feb 13, 2023

Uh oh!

atrick commented Feb 13, 2023

Uh oh!

atrick commented Feb 13, 2023

Uh oh!

lorentey commented Feb 13, 2023

Uh oh!

glessard left a comment

Uh oh!

glessard commented Feb 13, 2023

Uh oh!

atrick commented Feb 14, 2023

Uh oh!

lorentey commented Feb 14, 2023

Uh oh!

atrick commented Feb 17, 2023

Uh oh!

Uh oh!

Fix potentially undefined behavior in StringObject.nativeStorage and document Builtin.unsafeBitCast #63631

Fix potentially undefined behavior in StringObject.nativeStorage and document Builtin.unsafeBitCast #63631

Uh oh!

Conversation

atrick commented Feb 13, 2023

Uh oh!

atrick commented Feb 13, 2023

Uh oh!

atrick commented Feb 13, 2023

Uh oh!

lorentey commented Feb 13, 2023

Uh oh!

glessard left a comment

Choose a reason for hiding this comment

Uh oh!

glessard commented Feb 13, 2023

Performance (x86_64): -O

Code size: -O

Performance (x86_64): -Osize

Code size: -Osize

Performance (x86_64): -Onone

Code size: -swiftlibs

Uh oh!

atrick commented Feb 14, 2023

Uh oh!

lorentey commented Feb 14, 2023

Uh oh!

atrick commented Feb 17, 2023

Uh oh!

Uh oh!