Avoid attempting to create SmallStrings for constant tagged CFStrings #30966

Catfish-Man · 2020-04-10T23:41:24Z

Fixes rdar://problem/61273899

Catfish-Man · 2020-04-11T00:21:34Z

Catfish-Man · 2020-04-11T00:21:39Z

@swift-ci please benchmark

swift-ci · 2020-04-11T01:10:29Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
RemoveWhereMoveInts	33	36	+9.1%	0.92x (?)
ObjectiveCBridgeFromNSArrayAnyObjectForced	4240	4620	+9.0%	0.92x (?)
ObjectiveCBridgeStubFromNSDate	6050	6570	+8.6%	0.92x (?)
RemoveWhereSwapInts	59	64	+8.5%	0.92x (?)
Array2D	6944	7520	+8.3%	0.92x (?)
MapReduce	343	371	+8.2%	0.92x (?)
MapReduceClass2	37	40	+8.1%	0.93x
ArrayPlusEqualFiveElementCollection	7807	8436	+8.1%	0.93x (?)
ArrayInClass	1510	1625	+7.6%	0.93x (?)
DistinctClassFieldAccesses	302	325	+7.6%	0.93x (?)
MapReduceAnyCollection	369	397	+7.6%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
ArrayAppendLazyMap	7100	6140	-13.5%	1.16x (?)
LazilyFilteredArrayContains	41400	36600	-11.6%	1.13x
ObjectiveCBridgeStubToNSDate2	610	540	-11.5%	1.13x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
FlattenListLoop	3999	4363	+9.1%	0.92x (?)
RemoveWhereMoveInts	34	37	+8.8%	0.92x (?)
Array2D	6944	7520	+8.3%	0.92x (?)
ArrayPlusEqualFiveElementCollection	7696	8325	+8.2%	0.92x (?)
RemoveWhereSwapInts	62	67	+8.1%	0.93x (?)
ArrayInClass	1510	1625	+7.6%	0.93x (?)
DistinctClassFieldAccesses	303	326	+7.6%	0.93x

Improvement	OLD	NEW	DELTA	RATIO
ArrayAppendLazyMap	7900	6990	-11.5%	1.13x (?)
ObjectiveCBridgeFromNSSetAnyObjectToStringForced	96500	90000	-6.7%	1.07x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
StringToDataLargeUnicode	4650	5350	+15.1%	0.87x (?)
FloatingPointPrinting_Float_interpolated	71400	78200	+9.5%	0.91x (?)
DataReplaceLargeBuffer	32	35	+9.4%	0.91x (?)
ArrayOfPOD	1033	1114	+7.8%	0.93x (?)

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 Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

Catfish-Man · 2020-04-11T01:34:56Z

Excellent. Moving the slow path out of line for isEqual/isEqualToString didn't hurt anything :)

compnerd · 2020-04-13T17:41:37Z

stdlib/public/SwiftShims/CoreFoundationShims.h

+  unsigned long flags;
+  const __swift_uint8_t * _Nonnull str;
+  unsigned long length;
+} _swift_shims_builtin_CFString;


Does it matter that this type is mismatched on Linux? There are three different ABIs in play :-(:

Default (ObjC) (-fcf-runtime-abi=objc):

struct __NSConstantString_tag { const int *isa; int flags; const char *str; long length; } __NSConstantString;

Swift 4.1, 4.2 (-fcfc-runtime-abi=swift-4.1, -fcf-runtime-abi=swift-4.2):

typedef struct __NSConstantString_tag { uintptr_t _cfisa; uintptr_t _swift_rc; _Atomic(uint64_t) _cfinfoa; const char *_ptr; uint32_t _length; } __NSConstantString;

Swift 5.0 (-fcf-runtime=swift-5.0):

typedef struct __NSConstantString_tag { uintptr_t _cfisa; uintptr_t _swift_rc; _Atomic(uint64_t) _cfinfoa; const char *_ptr; uintptr_t _length; } __NSConstantString;

This is only intended for Darwin, but maybe there should be a guard here to make it explicit and prevent anyone from mistakenly using it more widely in the future.

I believe the CF shims file is already only built on Darwin, but thank you for pointing this out, it's good to be aware of.

If this is meant to be Darwin only, I think that we should do something to make that explicit (perhaps move the file) - as it is included in the Linux/Android/Windows SDKs.

milseman

Most requesting comments, simplifications, and clarifications

milseman · 2020-04-11T19:44:43Z

stdlib/public/core/StringBridge.swift

@@ -288,13 +288,24 @@ internal enum _KnownCocoaString {
 #if !(arch(i386) || arch(arm) || arch(wasm32))
  case tagged
 #endif
+#if arch(arm64)
+  case constantTagged


Is it that big a deal that this is separated out as only arm64? Or more broadly, do we want to diverge the code cross any platforms in the enum definition itself, or just at the create/use site? Even more broadly, do we want to diverge at the use site or only at the creation site?

We don't want to rely on the x86_64 tagged pointer format not accidentally overlapping the values for these, so I think it is unfortunately necessary for now

But why is that needed for the declaration of the type, in contrast to the code that chooses what case to construct?

milseman · 2020-04-11T19:47:18Z

stdlib/public/core/StringBridge.swift

@@ -288,13 +288,24 @@ internal enum _KnownCocoaString {
 #if !(arch(i386) || arch(arm) || arch(wasm32))
  case tagged
 #endif
+#if arch(arm64)
+  case constantTagged
+#endif

  @inline(__always)
  init(_ str: _CocoaString) {

 #if !(arch(i386) || arch(arm))


It's unfortunate that the wasm32 check isn't consistency enforced.

Maybe we could have a

private var platformSupportsObjCTaggedPointer: Bool { #if ... return true # else return false }

That would be nice!

milseman · 2020-04-11T19:48:27Z

stdlib/public/stubs/FoundationHelpers.mm

@@ -99,5 +99,10 @@ typedef __swift_uint8_t (*getCStringImplPtr)(id,

 }

+const void *


What is this and what is it for? Could you write a descriptive comment?

stdlib/public/SwiftShims/CoreFoundationShims.h

milseman · 2020-04-11T19:51:10Z

stdlib/public/core/StringStorageBridge.swift

+    otherIsKnownNSString: Bool,
+    maybeOtherUTF16Length: Int?,
+    maybeOtherASCIIPointer: UnsafePointer<UInt8>?
+  ) -> Int8 {


This is going to need way more comments and clarifications. It seems you joined together two code paths with some similarities, but added bool arguments relevant to some callers but not others.

This is a bit of an anti-pattern if you can avoid it. Instead, it seems like there are two very different ways to call this function, so is there a formulation of that that reflects that?

This used to be a fallthrough but I can't do two different fallthroughs. I can try to make this nicer, but I do want to keep the optimizations the arguments provide.

milseman · 2020-04-11T19:51:52Z

stdlib/public/core/StringStorageBridge.swift

+      return _isEqualSlowPath(
+        other: other,
+        otherIsKnownASCII: true,
+        otherIsKnownNSString: false,


E.g. why would a tagged Cocoa NSString* not be a known NSString?

If someone passed the wrong type to -isEqual:. We can only verify that the tag is correct on the new path currently.

milseman · 2020-04-11T19:53:03Z

stdlib/public/core/StringBridge.swift

@@ -358,6 +369,52 @@ private func _getCocoaStringPointer(
  return .none
 }

+#if arch(arm64)
+
+let taggedConstantStringsAvailable =


I believe this will have storage, is that your intent?

Yup, for the time being at least :/

milseman · 2020-04-11T19:54:34Z

stdlib/public/core/StringBridge.swift

+
+@inline(__always)
+internal func getConstantTaggedCocoaContents(_ cocoaString: _CocoaString) ->
+    (UTF16Length: Int, contentsPointer: UnsafePointer<UInt8>, isASCII: Bool)? {


I think you need this to be an UnsafeRawPointer, otherwise you're breaking type safety if you're going to have it be UIn8 for ASCII and UInt16 for non-ASCII. Or is it always in a single-byte representation?

Any chance you can reflect this in the type system? Would it makes sense for it to be an enum between a UBP<UInt8> or UBP<UInt16>? Is ASCII-ness orthogonal, or is that basically the discriminator?

Currently the plan is to ignore it completely for non-ASCII, so I suppose I could just represent it as a single optional pointer instead.

milseman · 2020-04-11T19:56:41Z

stdlib/public/core/StringBridge.swift

+    let taggedContents = getConstantTaggedCocoaContents(cocoaString)!
+    return _StringGuts(
+      cocoa: cocoaString,
+      providesFastUTF8: false, //TODO: if contentsPtr is UTF8 compatible, use it


Is that the same as isASCII?

Today, yes. I want to change that in the future, but that requires clang and CF changes, and may never happen.

milseman · 2020-04-13T21:45:15Z

@Catfish-Man could you add tests as well?

milseman · 2020-04-14T18:36:17Z

stdlib/public/core/StringStorageBridge.swift

+        otherIsKnownNSString: true,
+        maybeOtherUTF16Length: contents.UTF16Length,
+        maybeOtherASCIIPointer: contents.asciiContentsPointer
+      )


I still think there's an opportunity to simplify the _isEqualSlowPath's interface a bit. Having 2 bools and 2 optionals presents 16 ways to call this function, but not all combinations are possible.

For example, you can order the constantTagged case above .tagged and drop the otherIsKnownNSString parameter, since the fall-through shared by .tagged and .cocoa will have the _isNSString() call. I think you can do something similar for the ASCII-ness based on your earlier comment. Also, it seems like the two maybes are not orthogonal, so that's another factor of 2 you might be able to simplify.

edit: Actually, If I'm reading this right, .constantTagged always knows the length and the other two cases never do. So you can pass utf16Length as a non-optional and the fall-through calls _stdlib_binary_CFStringGetLength to derive that value.

I think the new version should make you happier here :)

Catfish-Man · 2020-04-14T21:04:13Z

@swift-ci please test

Catfish-Man · 2020-04-14T21:04:19Z

@swift-ci please benchmark

swift-ci · 2020-04-14T21:06:49Z

Build failed
Swift Test OS X Platform
Git Sha - 548f831b4ae42f26eb676d0f9109bf3a85f83384

swift-ci · 2020-04-14T21:06:58Z

Build failed
Swift Test Linux Platform
Git Sha - 548f831b4ae42f26eb676d0f9109bf3a85f83384

swift-ci · 2020-04-14T22:00:59Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
ArrayAppendLazyMap	6100	7110	+16.6%	0.86x (?)
ObjectiveCBridgeStringIsEqualAllSwift	77	88	+14.3%	0.88x (?)
LazilyFilteredArrayContains	36600	41400	+13.1%	0.88x

Improvement	OLD	NEW	DELTA	RATIO
StringToDataMedium	4050	3550	-12.3%	1.14x (?)
RandomShuffleLCG2	768	704	-8.3%	1.09x (?)
RemoveWhereMoveInts	36	33	-8.3%	1.09x (?)
ObjectiveCBridgeStubFromNSDate	6560	6040	-7.9%	1.09x (?)
RemoveWhereSwapInts	64	59	-7.8%	1.08x
Array2D	7520	6944	-7.7%	1.08x (?)
MapReduce	371	343	-7.5%	1.08x (?)
ArrayPlusEqualFiveElementCollection	8436	7807	-7.5%	1.08x (?)
ArrayInClass	1625	1510	-7.1%	1.08x (?)
DistinctClassFieldAccesses	325	302	-7.1%	1.08x (?)
MapReduceAnyCollection	397	369	-7.1%	1.08x (?)
FlattenListLoop	5284	4922	-6.9%	1.07x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
ArrayAppendLazyMap	7210	8130	+12.8%	0.89x (?)
ObjectiveCBridgeStringIsEqualAllSwift	78	87	+11.5%	0.90x (?)

Improvement	OLD	NEW	DELTA	RATIO
RemoveWhereMoveInts	37	34	-8.1%	1.09x (?)
Array2D	7520	6944	-7.7%	1.08x (?)
ArrayPlusEqualFiveElementCollection	8325	7696	-7.6%	1.08x (?)
RemoveWhereSwapInts	67	62	-7.5%	1.08x (?)
ArrayInClass	1625	1510	-7.1%	1.08x (?)
DistinctClassFieldAccesses	326	303	-7.1%	1.08x (?)
FlattenListLoop	5197	4837	-6.9%	1.07x (?)
MapReduceAnyCollection	433	404	-6.7%	1.07x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStringIsEqualAllSwift	78	89	+14.1%	0.88x
CSVParsingAltIndices2	9295	10505	+13.0%	0.88x (?)
DictionaryOfAnyHashableStrings_lookup	5064	5496	+8.5%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
ExclusivityIndependent	98	90	-8.2%	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 Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

Catfish-Man · 2020-04-14T22:03:14Z

Slightly sad that didn't show up as a win for existing tagged pointer stuff, but eh, I'll take no regression

Catfish-Man · 2020-04-14T22:27:05Z

@swift-ci please test

swift-ci · 2020-04-14T22:29:47Z

Build failed
Swift Test Linux Platform
Git Sha - da2f579d0359ef756d89615a91171fdcac63708a

swift-ci · 2020-04-14T22:30:31Z

Build failed
Swift Test OS X Platform
Git Sha - da2f579d0359ef756d89615a91171fdcac63708a

Catfish-Man · 2020-04-14T22:52:42Z

Known random failure apparently

Catfish-Man · 2020-04-14T22:52:45Z

@swift-ci please test

swift-ci · 2020-04-14T23:16:56Z

Build failed
Swift Test OS X Platform
Git Sha - a080111c219ba6d814135ba9d71367f7ca982c01

Catfish-Man · 2020-04-14T23:33:53Z

@swift-ci please test

swift-ci · 2020-04-14T23:36:38Z

Build failed
Swift Test Linux Platform
Git Sha - a080111c219ba6d814135ba9d71367f7ca982c01

swift-ci · 2020-04-14T23:46:38Z

Build failed
Swift Test OS X Platform
Git Sha - a080111c219ba6d814135ba9d71367f7ca982c01

milseman

Looking much better! My main remaining advice is to sink the architecture checks into getConstantTaggedCocoaContents so that we don't have to proliferate as many #if checks.

stdlib/public/core/StringBridge.swift

Catfish-Man · 2020-04-15T19:54:20Z

@swift-ci please test

swift-ci · 2020-04-15T19:57:04Z

Build failed
Swift Test Linux Platform
Git Sha - 4a875a87e30b372368583eb754beff609cb8e9e4

swift-ci · 2020-04-15T19:59:03Z

Build failed
Swift Test OS X Platform
Git Sha - 4a875a87e30b372368583eb754beff609cb8e9e4

milseman

LGTM, thanks for the cleanup!

Catfish-Man requested review from milseman and mikeash April 10, 2020 23:41

Catfish-Man marked this pull request as draft April 10, 2020 23:58

Catfish-Man marked this pull request as ready for review April 11, 2020 04:10

Catfish-Man self-assigned this Apr 11, 2020

compnerd reviewed Apr 13, 2020

View reviewed changes

milseman requested changes Apr 13, 2020

View reviewed changes

milseman reviewed Apr 14, 2020

View reviewed changes

milseman reviewed Apr 15, 2020

View reviewed changes

stdlib/public/core/StringBridge.swift Outdated Show resolved Hide resolved

stdlib/public/core/StringBridge.swift Outdated Show resolved Hide resolved

stdlib/public/core/StringBridge.swift Outdated Show resolved Hide resolved

Avoid attempting to create SmallStrings for constant tagged CFStrings

5ad4b15

Catfish-Man force-pushed the tagged-too branch from 4a875a8 to 5ad4b15 Compare April 15, 2020 19:30

milseman approved these changes Apr 15, 2020

View reviewed changes

Catfish-Man merged commit e192a7e into swiftlang:master Apr 15, 2020

		@@ -99,5 +99,10 @@ typedef __swift_uint8_t (*getCStringImplPtr)(id,

		}

		const void *

Avoid attempting to create SmallStrings for constant tagged CFStrings #30966

Avoid attempting to create SmallStrings for constant tagged CFStrings #30966

Uh oh!

Conversation

Catfish-Man commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Catfish-Man commented Apr 11, 2020

Uh oh!

Catfish-Man commented Apr 11, 2020

Uh oh!

swift-ci commented Apr 11, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

Catfish-Man commented Apr 11, 2020

Uh oh!

compnerd Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Catfish-Man Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milseman commented Apr 13, 2020

Uh oh!

milseman Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Apr 14, 2020

Uh oh!

Catfish-Man commented Apr 14, 2020

Catfish-Man commented Apr 10, 2020 •

edited

Loading

compnerd Apr 13, 2020 •

edited

Loading

Catfish-Man Apr 13, 2020 •

edited

Loading

milseman Apr 14, 2020 •

edited

Loading