[DNM] Experimentally change how Dictionary casting handles Strings, and speed up String bridging a bit to compensate #21066
Conversation
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke test |
|
We will see how this goes; local testing suggests it may still be a slowdown :( |
stdlib/public/core/String.swift
Outdated
| @@ -842,3 +842,9 @@ extension String { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| extension String { | |||
| public var _isASCII:Bool { | |||
There was a problem hiding this comment.
Please add an SPI note:
public // SPI(Foundation)
var ...Or "Bridging overlay" or whatever
There was a problem hiding this comment.
Nitpick: _isKnownASCII would be a better name, since this test is not guaranteed to return true for all ASCII strings.
Build comment file:Performance: -O
Performance: -Osize
Performance: -Onone
How to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
what |
|
quick merge it before it changes its mind |
| anyValue as AnyObject, Value.self) | ||
| if Key.self == String.self && | ||
| !(unsafeBitCast(key, to: String.self)._isASCII) { | ||
| maybeDict = builder.take() |
There was a problem hiding this comment.
This should be in the else branch below; otherwise this will call builder.take() for each non-ASCII key, trapping if there's more than one.
(This also applies for _conditionallyBridgeFromObjectiveC, below)
| let value = Swift._forceBridgeFromObjectiveC( | ||
| anyValue as AnyObject, Value.self) | ||
| if Key.self == String.self && | ||
| !(unsafeBitCast(key, to: String.self)._isASCII) { |
There was a problem hiding this comment.
_isNFC would potentially catch more cases -- although not necessarily today
| // `Dictionary<Key, Value>` where either `Key` or `Value` is a value type | ||
| // may not be backed by an NSDictionary. | ||
| var builder = _DictionaryBuilder<Key, Value>(count: x.count) | ||
| var maybeDict:Dictionary? = nil |
There was a problem hiding this comment.
Hm, I think we should rather extend the builder abstraction to support potentially non-unique insertions (via a new variant of the add method) -- so that we can interleave known-unique insertions with potential duplicates, and still get builder's performance benefits for all unique keys.
stdlib/public/core/String.swift
Outdated
| @@ -842,3 +842,9 @@ extension String { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| extension String { | |||
| public var _isASCII:Bool { | |||
There was a problem hiding this comment.
Nitpick: _isKnownASCII would be a better name, since this test is not guaranteed to return true for all ASCII strings.
| if _isBridgedVerbatimToObjectiveC(Key.self) && | ||
| _isBridgedVerbatimToObjectiveC(Value.self) { | ||
| result = Swift._dictionaryDownCastConditional(anyDict) | ||
| return result != nil | ||
| result = [Key : Value](_cocoaDictionary: x) |
There was a problem hiding this comment.
Ah, this explains the 10x speedup! Sadly, I'm afraid we'll need to get rid of it. 😭
The ObjectiveCBridgeFromNSDictionaryAnyObject benchmark measures NSDictionary → [NSString: NSNumber] casts, which are supposed to fail if there is even a single key or value that cannot be down-casted.
_dictionaryDownCastConditional laboriously checks this, while init(_cocoaDictionary:) assumes the contents are guaranteed to be of the correct types already.
I'd have expected more failures than these! Are the tests hiding in the full test suite, or do we need to add more bridging / down casting tests? @swift-ci test macOS platform |
|
Build failed |
It seems the latter. 😟 |
Catfish-Man
force-pushed
the
star-studded-cast
branch
2 times, most recently
from
0a9a89d to
d810ccf
Compare
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke test |
|
oh, conflicts, argh |
Catfish-Man
force-pushed
the
star-studded-cast
branch
from
d810ccf to
1d414ff
Compare
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke test |
|
@swift-ci please smoke benchmark |
Build comment file:Performance: -O
Performance: -Osize
Performance: -Onone
How to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
| // string-keyed NSDictionaries may generate key collisions when bridged | ||
| // over to Swift. See rdar://problem/35995647 | ||
| if Key.self == String.self && | ||
| !(unsafeBitCast(key, to: String.self)._isKnownNFC) { |
There was a problem hiding this comment.
I don't think this is the right check. Just because this key is NFC doesn't mean a previous one wasn't.
There was a problem hiding this comment.
I also don't think it's a good idea to codify that there are no other bridged types that behave differently between Objective-C and Swift.
EDIT: I got the polarity of the check backwards for this comment (though the previous one still applies).
There was a problem hiding this comment.
Ok. I wouldn't bother reviewing it right now, I just wanted to try stuff out (in this case including Karoy's suggestion). I will take this into account if I get something useful though
|
20% of the total time now is spent in swift_getGenericMetadata. That's really unfortunate :( |
Instead, calculate the size directly from the metadata for T.
…ed up String bridging a bit to compensate
…s a decent amount of time in swift_getGenericMetadata
…ll spends a decent amount of time in swift_getGenericMetadata" This reverts commit 2e325db26cb33b6c124ed7783fbc7c075e53d0ab.
Catfish-Man
force-pushed
the
star-studded-cast
branch
from
91d8ef2 to
c92cfd1
Compare
|
@swift-ci please benchmark |
Build comment file:Performance: -O
Performance: -Osize
Performance: -Onone
How to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
ObjectiveCBridgeFromNSDictionaryAnyObject improved by 1.34x at -Osize, but didn't change at -O or -Onone? That seems… a bit unlikely. |
|
@Catfish-Man Actually it's quite possible. At -Osize we outline copy/move/destroy operations, an one of my changes stops us from instantiating Optional metadata when calling these thunks. However it looks like overall this change is not a win with your bridging work so there's something else going on. It might be a generic type other than Optional that is being instantiated too. |
Instead, calculate the size directly from the metadata for T.
|
@swift-ci please smoke benchmark |
4 similar comments
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke benchmark |
Catfish-Man
changed the title
|
@swift-ci please benchmark |
Build comment file:Performance: -O
Performance: -Osize
Performance: -Onone
How to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
Build comment file:Performance: -O
Performance: -Osize
Performance: -Onone
How to read the dataThe 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 Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
The Dictionary part of this is superseded by #21235 |
In particular, make sure we handle duplicate keys due to differing notions of string equality