[stdlib] Rewriting native hashed collection indices #3046
Conversation
austinzheng
changed the title
austinzheng
force-pushed
the
az-dsindex
branch
4 times, most recently
from
d3c1623 to
17c8c54
Compare
|
Good news: the cursory benchmarks I ran indicate that there are pretty significant speedups when working with native dictionaries and sets. There is a small, unavoidable amount of slowdown when initializing an Objective-C collection bridged from a Swift collection, but that is a natural consequence of the implementation changes. Bad news: the Benchmarks binary segfaults on two tests, each of which involves force (not conditional) casting an I'll investigate when I have time and update this ticket with my findings. |
austinzheng
force-pushed
the
az-dsindex
branch
3 times, most recently
from
a3e41ac to
5bb7dad
Compare
| #else | ||
| expectEqual(8, sizeofValue(dict)) | ||
| expectEqual(40, sizeofValue(dict)) |
There was a problem hiding this comment.
Sorry, we really want dictionaries and sets to be word-sized.
|
@austinzheng Great work! @moiseev and I left a few comments (from my account), but the biggest ask is the following. Could you move This change would allow you to make the index a plain int in both representations. |
|
Thank you so much for reviewing this! I would be happy to make these changes. |
|
I'm still working on making changes. I'll let you know when the PR is ready to be re-reviewed. Because we want the size of Let me know if you have any concerns with that approach. |
|
@austinzheng You can put the necessary stored properties into the |
|
Quick question whenever you have time. I noticed in the existing Exactly what is the purpose of the
Many thanks in advance. EDIT: Does the fact that the property is marked |
|
I'm actually unsure -- I have a suspicion (the access goes through |
| /// This class exists for Objective-C bridging. It holds a reference to a | ||
| /// `_Native${Self}Storage`, and can be upcast to `NS${Self}` when | ||
| /// bridging is necessary. | ||
| final internal class _Native${Self}BridgingStorage<${TypeParametersDecl}> |
There was a problem hiding this comment.
Could you provide more information about why this separate class is needed? I have a suspicion that allocating a new instance of it every time might be a cause of the performance regression.
There might be a correctness issue as well, some Objective-C code does not work well if the same Swift Dictionary is bridged to a fresh pointer every time, because there is code out there that relies on pointer identity. It is broken, but we have binary compatibility constraints, and everything just works better if we can guarantee a stable pointer.
Did you consider making the StorageImpl type conform to the _SwiftNativeNS${Self} protocols?
There was a problem hiding this comment.
This is a good idea. I'll look into having the native storage just serve as the bridged storage as well.
There was a problem hiding this comment.
@gribozavr, I see at least two possible complications:
_SwiftNativeNS${Self}is a base class defined in the runtime. It won't be possible to have the storage impl inherit from bothManagedBufferand that base class. (_NS${Self}Coreis the shadow protocol that provides the functionality.)_SwiftNativeNS${Self}is only available when the Objective-C runtime is available.
Basically, the bridging storage needs to be cast-able to a NSSet or NSDictionary, and it needs space for _heapBufferBridged_DoNotUse, neither of which can be provided by a ManagedBuffer as far as I know.
The original double indirection implementation sidestepped the issue by having the storage owner (first indirection) serve as the bridged storage, and separating that from the managed buffer (second indirection).
I'll keep thinking about this. Worst comes to worst I can restore the double indirection; since the indices no longer refer to either object the data race bug that was mentioned in SE-0065 should at least not be an issue.
There was a problem hiding this comment.
Good points. I'd recommend looking at how Array does this, it definitely only does one allocation, which is a subclass of NSArray. I think there's a ManagedBuffer initializer that takes another class type to allocate.
There was a problem hiding this comment.
Sorry for a terse reply, here are some more details that I hope are helpful.
The Array does this trick in ContiguousArrayBuffer.swift, in _ContiguousArrayBuffer.init(uninitializedCount: Int, minimumCapacity: Int). It creates an instance of _ContiguousArrayStorage (which is a subclass of _SwiftNativeNSArray) using ManagedBufferPointer that allows it to get the tail-allocation.
In the non-Objective-C runtime we just define an empty _SwiftNativeNSArrayWithContiguousStorage class, but still go through ManagedBufferPointer to get the tail-allocation.
Does this make sense?
There was a problem hiding this comment.
No worries, I just haven't had a chance to explore the Array code yet. What you say makes perfect sense; I'm hoping to rewrite the buffer this weekend or next week and resubmit for review. I'll let you know if I run into any issues. Thanks for the tips!
Build comment file:Optimized (O)
|
| TEST | OLD_MIN | NEW_MIN | DELTA (%) | SPEEDUP |
|---|---|---|---|---|
| ObjectiveCBridgeToNSDictionary | 16598 | 79672 | +380.0% | 0.21x |
| ObjectiveCBridgeToNSSet | 16321 | 63889 | +291.4% | 0.26x |
| DictionaryLiteral | 4362 | 9669 | +121.7% | 0.45x |
| NSDictionaryCastToSwift | 13487 | 26713 | +98.1% | 0.50x |
| ObjectiveCBridgeFromNSDictionaryAnyObject | 245745 | 389058 | +58.3% | 0.63x |
| ObjectiveCBridgeFromNSSetAnyObject | 111758 | 157802 | +41.2% | 0.71x |
| ObjectiveCBridgeFromNSSetAnyObjectForced | 4071 | 5138 | +26.2% | 0.79x |
| ObjectiveCBridgeFromNSSetAnyObjectToStringForced | 121679 | 153019 | +25.8% | 0.80x |
| ObjectiveCBridgeFromNSDictionaryAnyObjectForced | 4668 | 5664 | +21.3% | 0.82x |
| TwoSum | 1936 | 2368 | +22.3% | 0.82x |
| ObjectiveCBridgeFromNSDictionaryAnyObjectToString | 216078 | 256300 | +18.6% | 0.84x |
| ObjectiveCBridgeFromNSDictionaryAnyObjectToStringForced | 144740 | 171936 | +18.8% | 0.84x |
| ObjectiveCBridgeFromNSSetAnyObjectToString | 164544 | 196060 | +19.1% | 0.84x |
| DictionaryBridge | 3874 | 4354 | +12.4% | 0.89x |
| 133 | 3668409 | 4104772 | +11.9% | 0.89x |
| ObjectiveCBridgeStubNSDateRefAccess | 307 | 334 | +8.8% | 0.92x(?) |
Improvement (15)
| TEST | OLD_MIN | NEW_MIN | DELTA (%) | SPEEDUP |
|---|---|---|---|---|
| ClassArrayGetter | 12 | 11 | -8.3% | 1.09x(?) |
| Dictionary3 | 546 | 491 | -10.1% | 1.11x |
| SetUnion_OfObjects | 7045 | 6248 | -11.3% | 1.13x |
| DictionaryRemoveOfObjects | 19986 | 17510 | -12.4% | 1.14x |
| Sim2DArray | 438 | 384 | -12.3% | 1.14x |
| SetExclusiveOr | 3765 | 3033 | -19.4% | 1.24x |
| SetExclusiveOr_OfObjects | 8341 | 6625 | -20.6% | 1.26x |
| SetUnion | 3308 | 2442 | -26.2% | 1.35x |
| DictionarySwap | 788 | 577 | -26.8% | 1.37x |
| DictionaryRemove | 4976 | 3644 | -26.8% | 1.37x |
| Histogram | 662 | 334 | -49.5% | 1.98x |
| SetIsSubsetOf | 489 | 247 | -49.5% | 1.98x |
| SetIsSubsetOf_OfObjects | 615 | 307 | -50.1% | 2.00x |
| SetIntersect_OfObjects | 2354 | 960 | -59.2% | 2.45x |
| SetIntersect | 1172 | 469 | -60.0% | 2.50x |
No Changes (103)
| TEST | OLD_MIN | NEW_MIN | DELTA (%) | SPEEDUP |
|---|---|---|---|---|
| StringHasSuffix | 731 | 694 | -5.1% | 1.05x |
| ObjectiveCBridgeStubFromNSString | 678 | 647 | -4.6% | 1.05x |
| ProtocolDispatch2 | 161 | 154 | -4.3% | 1.05x |
| CaptureProp | 4397 | 4200 | -4.5% | 1.05x |
| StackPromo | 19764 | 18966 | -4.0% | 1.04x |
| Dictionary3OfObjects | 891 | 856 | -3.9% | 1.04x(?) |
| Walsh | 340 | 326 | -4.1% | 1.04x(?) |
| Prims | 759 | 739 | -2.6% | 1.03x |
| Integrate | 243 | 236 | -2.9% | 1.03x(?) |
| StringHasSuffixUnicode | 65490 | 63690 | -2.8% | 1.03x(?) |
| ObjectiveCBridgeStubURLAppendPathRef | 206195 | 200834 | -2.6% | 1.03x(?) |
| Calculator | 34 | 33 | -2.9% | 1.03x |
| Dictionary | 745 | 721 | -3.2% | 1.03x |
| ObjectiveCBridgeFromNSArrayAnyObject | 65740 | 63847 | -2.9% | 1.03x(?) |
| RGBHistogram | 3146 | 3065 | -2.6% | 1.03x(?) |
| PopFrontArray | 1146 | 1124 | -1.9% | 1.02x(?) |
| ObjectiveCBridgeStubToNSString | 1276 | 1248 | -2.2% | 1.02x(?) |
| ObjectiveCBridgeFromNSArrayAnyObjectForced | 3790 | 3712 | -2.1% | 1.02x(?) |
| ObjectiveCBridgeFromNSStringForced | 2312 | 2265 | -2.0% | 1.02x(?) |
| ObjectiveCBridgeStubFromNSStringRef | 131 | 129 | -1.5% | 1.02x(?) |
| PopFrontArrayGeneric | 1154 | 1132 | -1.9% | 1.02x(?) |
| MapReduce | 376 | 369 | -1.9% | 1.02x(?) |
| ObjectiveCBridgeFromNSString | 1614 | 1586 | -1.7% | 1.02x(?) |
| Chars | 674 | 659 | -2.2% | 1.02x(?) |
| DeadArray | 179 | 176 | -1.7% | 1.02x |
| Dictionary2OfObjects | 3338 | 3272 | -2.0% | 1.02x(?) |
| ObjectiveCBridgeStubFromNSDate | 3598 | 3532 | -1.8% | 1.02x(?) |
| ObjectiveCBridgeToNSString | 1056 | 1048 | -0.8% | 1.01x(?) |
| ObjectiveCBridgeFromNSArrayAnyObjectToString | 171956 | 170533 | -0.8% | 1.01x(?) |
| DictionarySwapOfObjects | 6593 | 6552 | -0.6% | 1.01x(?) |
| RangeAssignment | 316 | 312 | -1.3% | 1.01x(?) |
| ObjectAllocation | 146 | 144 | -1.4% | 1.01x(?) |
| ObjectiveCBridgeStubDateMutation | 267 | 264 | -1.1% | 1.01x(?) |
| NSError | 326 | 323 | -0.9% | 1.01x(?) |
| NopDeinit | 35876 | 35633 | -0.7% | 1.01x(?) |
| ArrayOfGenericRef | 3592 | 3554 | -1.1% | 1.01x(?) |
| Phonebook | 7214 | 7140 | -1.0% | 1.01x(?) |
| HashTest | 2003 | 1974 | -1.4% | 1.01x(?) |
| ArrayAppend | 740 | 732 | -1.1% | 1.01x(?) |
| ObjectiveCBridgeStubNSDataAppend | 2351 | 2324 | -1.1% | 1.01x(?) |
| MonteCarloPi | 42519 | 42459 | -0.1% | 1.00x(?) |
| RecursiveOwnedParameter | 1833 | 1832 | -0.1% | 1.00x(?) |
| StringWithCString | 587269 | 587523 | +0.0% | 1.00x |
| SortLettersInPlace | 957 | 958 | +0.1% | 1.00x(?) |
| ByteSwap | 1 | 1 | +0.0% | 1.00x |
| XorLoop | 359 | 360 | +0.3% | 1.00x(?) |
| StaticArray | 2803 | 2797 | -0.2% | 1.00x(?) |
| ProtocolDispatch | 2968 | 2981 | +0.4% | 1.00x(?) |
| TypeFlood | 0 | 0 | +0.0% | 1.00x |
| AngryPhonebook | 2859 | 2848 | -0.4% | 1.00x(?) |
| ErrorHandling | 3045 | 3054 | +0.3% | 1.00x(?) |
| OpenClose | 53 | 53 | +0.0% | 1.00x |
| PolymorphicCalls | 61 | 61 | +0.0% | 1.00x |
| Hanoi | 3320 | 3321 | +0.0% | 1.00x(?) |
| ArrayOfGenericPOD | 208 | 208 | +0.0% | 1.00x |
| ObjectiveCBridgeStubToArrayOfNSString | 31044 | 31191 | +0.5% | 1.00x(?) |
| ObjectiveCBridgeStubDataAppend | 2907 | 2906 | -0.0% | 1.00x(?) |
| RGBHistogramOfObjects | 21860 | 21851 | -0.0% | 1.00x(?) |
| StringBuilder | 1524 | 1527 | +0.2% | 1.00x(?) |
| ObjectiveCBridgeStubDateAccess | 174 | 174 | +0.0% | 1.00x |
| MonteCarloE | 10058 | 10040 | -0.2% | 1.00x |
| GlobalClass | 0 | 0 | +0.0% | 1.00x |
| StringHasPrefixUnicode | 15663 | 15630 | -0.2% | 1.00x(?) |
| LinkedList | 6821 | 6809 | -0.2% | 1.00x(?) |
| ArrayAppendReserved | 518 | 516 | -0.4% | 1.00x |
| StrToInt | 5034 | 5020 | -0.3% | 1.00x(?) |
| ArrayValueProp2 | 5 | 5 | +0.0% | 1.00x |
| BitCount | 1 | 1 | +0.0% | 1.00x |
| ArrayLiteral | 1009 | 1014 | +0.5% | 1.00x(?) |
| ArrayValueProp3 | 5 | 5 | +0.0% | 1.00x |
| StringWalk | 6204 | 6192 | -0.2% | 1.00x(?) |
| ArrayValueProp | 5 | 5 | +0.0% | 1.00x |
| ObjectiveCBridgeStubNSDateMutationRef | 12593 | 12550 | -0.3% | 1.00x(?) |
| RC4 | 253 | 253 | +0.0% | 1.00x |
| Memset | 222 | 222 | +0.0% | 1.00x |
| ArrayValueProp4 | 5 | 5 | +0.0% | 1.00x |
| SevenBoom | 1310 | 1306 | -0.3% | 1.00x(?) |
| Array2D | 2007 | 2023 | +0.8% | 0.99x(?) |
| SortStrings | 1758 | 1772 | +0.8% | 0.99x(?) |
| ObjectiveCBridgeStubFromNSDateRef | 3631 | 3683 | +1.4% | 0.99x(?) |
| Join | 454 | 457 | +0.7% | 0.99x(?) |
| Dictionary2 | 1918 | 1934 | +0.8% | 0.99x(?) |
| DictionaryOfObjects | 2238 | 2253 | +0.7% | 0.99x(?) |
| PopFrontUnsafePointer | 8877 | 8999 | +1.4% | 0.99x(?) |
| StringEqualPointerComparison | 7398 | 7480 | +1.1% | 0.99x(?) |
| SortStringsUnicode | 9232 | 9340 | +1.2% | 0.99x(?) |
| ArrayInClass | 84 | 85 | +1.2% | 0.99x(?) |
| ArrayOfPOD | 171 | 172 | +0.6% | 0.99x |
| ArraySubscript | 1347 | 1368 | +1.6% | 0.98x |
| SuperChars | 356997 | 364539 | +2.1% | 0.98x |
| StringInterpolation | 11333 | 11533 | +1.8% | 0.98x(?) |
| StrComplexWalk | 3127 | 3199 | +2.3% | 0.98x |
| ArrayOfRef | 3412 | 3495 | +2.4% | 0.98x(?) |
| ObjectiveCBridgeStubToNSStringRef | 116 | 118 | +1.7% | 0.98x |
| UTF8Decode | 314 | 322 | +2.5% | 0.98x |
| ObjectiveCBridgeStubURLAppendPath | 197211 | 201504 | +2.2% | 0.98x(?) |
| StringHasPrefix | 649 | 669 | +3.1% | 0.97x(?) |
| ObjectiveCBridgeStubToNSDate | 13264 | 13694 | +3.2% | 0.97x(?) |
| ObjectiveCBridgeToNSArray | 31700 | 32557 | +2.7% | 0.97x |
| ObjectiveCBridgeStubToNSDateRef | 3264 | 3377 | +3.5% | 0.97x |
| NSStringConversion | 589 | 609 | +3.4% | 0.97x |
| ObjectiveCBridgeFromNSArrayAnyObjectToStringForced | 167388 | 174213 | +4.1% | 0.96x(?) |
| ObjectiveCBridgeStubFromArrayOfNSString | 108895 | 114738 | +5.4% | 0.95x(?) |
Regression (44)
| TEST | OLD_MIN | NEW_MIN | DELTA (%) | SPEEDUP |
|---|---|---|---|---|
| ObjectiveCBridgeToNSDictionary | 16835 | 82308 | +388.9% | 0.20x |
| ObjectiveCBridgeToNSSet | 17043 | 66690 | +291.3% | 0.26x |
| DictionaryRemove | 31559 | 105622 | +234.7% | 0.30x |
| DictionarySwap | 11073 | 25955 | +134.4% | 0.43x |
| DictionaryRemoveOfObjects | 61819 | 142269 | +130.1% | 0.43x |
| SetUnion | 31955 | 74256 | +132.4% | 0.43x |
| TwoSum | 9629 | 21206 | +120.2% | 0.45x |
| SetExclusiveOr | 46491 | 100262 | +115.7% | 0.46x |
| StringWalk | 23764 | 50098 | +110.8% | 0.47x |
| NSDictionaryCastToSwift | 14451 | 28633 | +98.1% | 0.50x |
| SetUnion_OfObjects | 49497 | 95644 | +93.2% | 0.52x |
| SetExclusiveOr_OfObjects | 65702 | 123972 | +88.7% | 0.53x |
| Dictionary | 2885 | 5267 | +82.6% | 0.55x |
| DictionaryLiteral | 24517 | 42228 | +72.2% | 0.58x |
| SetIntersect_OfObjects | 19323 | 32819 | +69.8% | 0.59x |
| SetIntersect | 19728 | 33104 | +67.8% | 0.60x |
| Dictionary3 | 2078 | 3428 | +65.0% | 0.61x |
| DictionarySwapOfObjects | 25593 | 41606 | +62.6% | 0.62x |
| ObjectiveCBridgeFromNSDictionaryAnyObject | 250046 | 397868 | +59.1% | 0.63x |
| Dictionary2 | 5231 | 7933 | +51.6% | 0.66x |
| Dictionary3OfObjects | 2716 | 4030 | +48.4% | 0.67x |
| DictionaryOfObjects | 5662 | 8277 | +46.2% | 0.68x |
| RGBHistogram | 51660 | 76060 | +47.2% | 0.68x |
| Prims | 13976 | 19752 | +41.3% | 0.71x |
| ObjectiveCBridgeFromNSSetAnyObject | 115494 | 163213 | +41.3% | 0.71x |
| Dictionary2OfObjects | 7016 | 9779 | +39.4% | 0.72x |
| SetIsSubsetOf | 3044 | 4173 | +37.1% | 0.73x |
| SetIsSubsetOf_OfObjects | 2924 | 4010 | +37.1% | 0.73x |
| Histogram | 13877 | 18851 | +35.8% | 0.74x |
| ObjectiveCBridgeFromNSDictionaryAnyObjectToStringForced | 142338 | 178884 | +25.7% | 0.80x |
| RGBHistogramOfObjects | 100810 | 126660 | +25.6% | 0.80x |
| ObjectiveCBridgeFromNSSetAnyObjectToStringForced | 131213 | 160913 | +22.6% | 0.82x |
| ObjectiveCBridgeFromNSSetAnyObjectToString | 178465 | 214432 | +20.1% | 0.83x |
| ObjectiveCBridgeFromNSDictionaryAnyObjectToString | 228853 | 269800 | +17.9% | 0.85x |
| OpenClose | 454 | 528 | +16.3% | 0.86x |
| 133 | 6649450 | 7619165 | +14.6% | 0.87x |
| ObjectiveCBridgeFromNSDictionaryAnyObjectForced | 7426 | 8395 | +13.1% | 0.88x(?) |
| ProtocolDispatch | 5501 | 6255 | +13.7% | 0.88x |
| ObjectiveCBridgeFromNSSetAnyObjectForced | 7786 | 8821 | +13.3% | 0.88x |
| NopDeinit | 57348 | 63523 | +10.8% | 0.90x(?) |
| DictionaryBridge | 4032 | 4359 | +8.1% | 0.92x |
| StrComplexWalk | 8060 | 8752 | +8.6% | 0.92x |
| ObjectiveCBridgeFromNSArrayAnyObjectToString | 166300 | 176362 | +6.0% | 0.94x(?) |
| ObjectiveCBridgeStubURLAppendPathRef | 203049 | 215003 | +5.9% | 0.94x(?) |
Improvement (1)
| TEST | OLD_MIN | NEW_MIN | DELTA (%) | SPEEDUP |
|---|---|---|---|---|
| PopFrontUnsafePointer | 255076 | 239581 | -6.1% | 1.06x |
No Changes (89)
| TEST | OLD_MIN | NEW_MIN | DELTA (%) | SPEEDUP |
|---|---|---|---|---|
| TypeFlood | 159 | 152 | -4.4% | 1.05x(?) |
| StringEqualPointerComparison | 9944 | 9486 | -4.6% | 1.05x(?) |
| ArrayOfPOD | 2392 | 2268 | -5.2% | 1.05x |
| ObjectiveCBridgeStubDateMutation | 492 | 471 | -4.3% | 1.04x(?) |
| ObjectiveCBridgeFromNSString | 4988 | 4864 | -2.5% | 1.03x(?) |
| ArrayValueProp | 2312 | 2253 | -2.5% | 1.03x |
| SortStrings | 2643 | 2594 | -1.9% | 1.02x(?) |
| ObjectiveCBridgeFromNSStringForced | 2744 | 2689 | -2.0% | 1.02x(?) |
| ObjectAllocation | 549 | 538 | -2.0% | 1.02x |
| PolymorphicCalls | 1184 | 1165 | -1.6% | 1.02x(?) |
| NSStringConversion | 2689 | 2640 | -1.8% | 1.02x(?) |
| ObjectiveCBridgeFromNSArrayAnyObjectToStringForced | 172346 | 168927 | -2.0% | 1.02x(?) |
| ObjectiveCBridgeStubFromArrayOfNSString | 115302 | 113391 | -1.7% | 1.02x(?) |
| ObjectiveCBridgeFromNSArrayAnyObjectForced | 7311 | 7233 | -1.1% | 1.01x(?) |
| Join | 1460 | 1444 | -1.1% | 1.01x(?) |
| ObjectiveCBridgeStubFromNSStringRef | 166 | 165 | -0.6% | 1.01x(?) |
| ArrayLiteral | 1090 | 1077 | -1.2% | 1.01x(?) |
| ArrayOfRef | 9071 | 8985 | -0.9% | 1.01x(?) |
| PopFrontArrayGeneric | 11258 | 11151 | -0.9% | 1.01x(?) |
| ObjectiveCBridgeFromNSArrayAnyObject | 69971 | 69573 | -0.6% | 1.01x(?) |
| ObjectiveCBridgeStubDataAppend | 3295 | 3248 | -1.4% | 1.01x(?) |
| ArrayInClass | 3676 | 3653 | -0.6% | 1.01x(?) |
| HashTest | 5474 | 5418 | -1.0% | 1.01x(?) |
| ArrayAppend | 3349 | 3328 | -0.6% | 1.01x(?) |
| Chars | 5456 | 5384 | -1.3% | 1.01x(?) |
| RC4 | 8583 | 8527 | -0.7% | 1.01x(?) |
| ArrayValueProp4 | 2702 | 2684 | -0.7% | 1.01x |
| ArrayValueProp3 | 2733 | 2708 | -0.9% | 1.01x(?) |
| ArraySubscript | 5345 | 5332 | -0.2% | 1.00x(?) |
| ObjectiveCBridgeToNSString | 1118 | 1116 | -0.2% | 1.00x(?) |
| MonteCarloPi | 50673 | 50578 | -0.2% | 1.00x(?) |
| StackPromo | 132370 | 131810 | -0.4% | 1.00x(?) |
| RecursiveOwnedParameter | 7595 | 7611 | +0.2% | 1.00x(?) |
| ClassArrayGetter | 1184 | 1182 | -0.2% | 1.00x(?) |
| Array2D | 768196 | 766045 | -0.3% | 1.00x |
| StringWithCString | 732848 | 730133 | -0.4% | 1.00x |
| ByteSwap | 10 | 10 | +0.0% | 1.00x |
| SuperChars | 533247 | 535649 | +0.5% | 1.00x(?) |
| ObjectiveCBridgeStubToNSDate | 13595 | 13570 | -0.2% | 1.00x(?) |
| XorLoop | 18611 | 18576 | -0.2% | 1.00x(?) |
| Walsh | 12371 | 12314 | -0.5% | 1.00x |
| ErrorHandling | 3834 | 3844 | +0.3% | 1.00x(?) |
| NSError | 668 | 671 | +0.5% | 1.00x(?) |
| ObjectiveCBridgeStubToNSStringRef | 155 | 155 | +0.0% | 1.00x |
| MapReduce | 43040 | 42988 | -0.1% | 1.00x(?) |
| ArrayOfGenericPOD | 3434 | 3425 | -0.3% | 1.00x(?) |
| UTF8Decode | 41743 | 41558 | -0.4% | 1.00x(?) |
| GlobalClass | 0 | 0 | +0.0% | 1.00x |
| ArrayOfGenericRef | 9752 | 9710 | -0.4% | 1.00x(?) |
| Sim2DArray | 14075 | 14060 | -0.1% | 1.00x(?) |
| MonteCarloE | 104258 | 103873 | -0.4% | 1.00x(?) |
| StringHasSuffixUnicode | 66163 | 66261 | +0.1% | 1.00x(?) |
| AngryPhonebook | 3662 | 3679 | +0.5% | 1.00x(?) |
| Memset | 19682 | 19778 | +0.5% | 1.00x(?) |
| ObjectiveCBridgeStubNSDataAppend | 5184 | 5181 | -0.1% | 1.00x(?) |
| ObjectiveCBridgeStubFromNSDateRef | 3995 | 4025 | +0.8% | 0.99x(?) |
| SortLettersInPlace | 2534 | 2565 | +1.2% | 0.99x(?) |
| Integrate | 365 | 367 | +0.6% | 0.99x(?) |
| ProtocolDispatch2 | 440 | 443 | +0.7% | 0.99x(?) |
| ObjectiveCBridgeToNSArray | 31798 | 32101 | +0.9% | 0.99x(?) |
| ObjectiveCBridgeStubToNSDateRef | 3192 | 3223 | +1.0% | 0.99x(?) |
| Hanoi | 19491 | 19682 | +1.0% | 0.99x(?) |
| SortStringsUnicode | 10384 | 10524 | +1.4% | 0.99x(?) |
| ObjectiveCBridgeStubToArrayOfNSString | 30848 | 31094 | +0.8% | 0.99x(?) |
| StringBuilder | 2854 | 2888 | +1.2% | 0.99x(?) |
| ObjectiveCBridgeStubDateAccess | 1090 | 1101 | +1.0% | 0.99x |
| StringHasPrefixUnicode | 16956 | 17116 | +0.9% | 0.99x(?) |
| LinkedList | 26138 | 26367 | +0.9% | 0.99x(?) |
| DeadArray | 132479 | 133371 | +0.7% | 0.99x(?) |
| ArrayValueProp2 | 2797 | 2822 | +0.9% | 0.99x(?) |
| BitCount | 95 | 96 | +1.1% | 0.99x |
| SevenBoom | 1467 | 1478 | +0.8% | 0.99x(?) |
| RangeAssignment | 23484 | 24076 | +2.5% | 0.98x |
| StringInterpolation | 16078 | 16369 | +1.8% | 0.98x |
| ObjectiveCBridgeStubFromNSString | 693 | 707 | +2.0% | 0.98x(?) |
| ObjectiveCBridgeStubURLAppendPath | 195131 | 198129 | +1.5% | 0.98x(?) |
| Phonebook | 60769 | 62244 | +2.4% | 0.98x(?) |
| ObjectiveCBridgeStubNSDateMutationRef | 14140 | 14388 | +1.8% | 0.98x(?) |
| StrToInt | 5421 | 5505 | +1.6% | 0.98x(?) |
| ObjectiveCBridgeStubFromNSDate | 3877 | 3942 | +1.7% | 0.98x(?) |
| CaptureProp | 114188 | 117247 | +2.7% | 0.97x(?) |
| PopFrontArray | 23921 | 24859 | +3.9% | 0.96x |
| ObjectiveCBridgeStubToNSString | 1293 | 1345 | +4.0% | 0.96x(?) |
| Calculator | 934 | 970 | +3.9% | 0.96x |
| StaticArray | 27942 | 29364 | +5.1% | 0.95x |
| StringHasPrefix | 1493 | 1573 | +5.4% | 0.95x(?) |
| ObjectiveCBridgeStubNSDateRefAccess | 1200 | 1264 | +5.3% | 0.95x(?) |
| ArrayAppendReserved | 3114 | 3269 | +5.0% | 0.95x(?) |
| StringHasSuffix | 1513 | 1599 | +5.7% | 0.95x |
| return unsafeBitCast(_variantStorage.asNative.buffer, to: Int.self) | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This extension can be moved to the unit test itself, I think. You can just bitcast the self value.
|
Just an idea: in the interest of getting the patch merged faster, and not having to iterate on a 2kloc patch, please feel free to submit the I'm OK with the large patch, but I can imagine it can be painful to carry it around. |
austinzheng
force-pushed
the
az-dsindex
branch
2 times, most recently
from
abcfeca to
149a6c7
Compare
austinzheng
force-pushed
the
az-dsindex
branch
2 times, most recently
from
d389505 to
e72efdb
Compare
Changes: - Native dictionary and set indices no longer hold references to storage - Cocoa-based dictionary and set indices no longer hold references to storage - Removed double indirection trick from hashed collections - Rewrote storage types to reflect simpler model - Updated unit tests
|
I haven't dropped this; with the |
|
Thank you @austinzheng! |
|
@austinzheng Now that process arguments is not using boxing could you move that into HashedCollections.swift.gyb please? |
|
Yeah, definitely. I'll pick this up again this week. On Thu, Jul 14, 2016 at 11:26 AM, Robert Widmann [email protected]
|
|
@austinzheng Could you actually Put |
|
I plan to pick this up again this week. However, I think it should stay out of Swift 3.0; merging in the completed work after 3.0 is cut gives it time to "cook" and be validated. Sorry for dropping this; life and work got in the way. |
|
@austinzheng are you still hoping to work on this, or would you like to pass this off to someone else? I'm currently looking into removing lazy bridging, and there's a pretty big conflict with all the refactoring you're doing here. I'm happy to do whatever to accomodate this (great!) work landing as painlessly as possible, up to and including rebasing and finishing it myself if that works best for you. |
|
Hey @gankro, Thanks for asking. I haven't lost interest, but have been too busy at work for the past few months to pick this up 😿 . If you think it would be fun you're welcome to take over; otherwise I'm willing to sit down this weekend for a few hours and try to finish it up if that'll unblock you. Let me know what you think. |
|
I'm happy to take the burden off ya, I'll get started on rebasing this now. |
|
Sweet, thanks. If you could assign yourself that would be great. @gribozavr is your main point of contact. If I remember correctly the Swift versions of the collections were working fine, but there was some refactoring needed to get ObjC bridging to not be broken. Let me know if you have any questions. |
|
Status update: been working on this this week. The rebase was... very ugly. Lots of conflicting changes. I'm currently trying to debug what seems to be undefined behaviour problems, presumably from the butchered merge git produced. Currently I'm seeing troubling behaviour where an ImplicitlyUnwrappedOptional value which seems to be always initialized to a .some is crashing as containing |
|
Ok I found the last error, the inconsistencies were the result of a badly cached build. The only outstanding problem is that the new bridging implementation seems to be breaking equality comparisons for I'm guessing this is because this kind of Dictionary doesn't provide its own equality comparison, that it's falling back to some object identity stuff, which is no longer holding? Not sure what guarantees we're providing here, or how to proceed. @moiseev any idea what the right solution is? |
|
@dabrahams any thoughts on what the correct behaviour/solution is? (with regards to my previous comment) |
|
Closing in favour of #5291. Thanks a lot for the great work, @austinzheng! |
What's in this pull request?
@gribozavr @moiseev
This PR refactors
Dictionary's andSet's native implementations in the following ways:Int.Dictionarys andSets.NS*-bridging class for interop purposes.It does not touch the
cocoa(CocoaStorage)variant of the Swift collection storage. A future PR will handle porting those over to fully take advantage of the new indices model.All tests pass on x64 (OS X) and Ubuntu.
Please feel free to contact me with any questions or concerns.
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.
Changes: