Skip to content

[stdlib] add withContiguousStorage to CollectionOfOne #66562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oxy
Copy link
Contributor

@oxy oxy commented Jun 12, 2023

As it says on the tin - conform to withContiguousStorageIfAvailable in CollectionOfOne. Enables (in theory) being able to take fast paths on certain operations.

@oxy
Copy link
Contributor Author

oxy commented Jun 12, 2023

@swift-ci please test

@oxy oxy force-pushed the cofone-contiguous branch from b135dee to 7722c84 Compare June 12, 2023 22:21
@oxy
Copy link
Contributor Author

oxy commented Jun 12, 2023

@swift-ci please test

@oxy
Copy link
Contributor Author

oxy commented Jun 16, 2023

@swift-ci please smoke test

@oxy
Copy link
Contributor Author

oxy commented Jun 17, 2023

Failed Tests (13):
Swift(macosx-x86_64) :: Interop/Cxx/stdlib/overlay/std-string-overlay.swift
Swift(macosx-x86_64) :: Prototypes/UnicodeDecoders.swift
Swift(macosx-x86_64) :: api-digester/stability-stdlib-abi-with-asserts.test
Swift(macosx-x86_64) :: stdlib/NSSlowString.swift
Swift(macosx-x86_64) :: stdlib/SmallString.swift
Swift(macosx-x86_64) :: stdlib/StringAPI.swift
Swift(macosx-x86_64) :: stdlib/StringBridge.swift
Swift(macosx-x86_64) :: stdlib/StringCreate.swift
Swift(macosx-x86_64) :: stdlib/StringIndex.swift
Swift-validation(macosx-x86_64) :: stdlib/String.swift
Swift-validation(macosx-x86_64) :: stdlib/StringUTF8.swift
Swift-validation(macosx-x86_64) :: stdlib/Unicode.swift.gyb
Swift-validation(macosx-x86_64) :: stdlib/UnicodeUTFEncoders.swift

@oxy oxy force-pushed the cofone-contiguous branch from 7722c84 to 21065a5 Compare August 10, 2023 18:42
@oxy
Copy link
Contributor Author

oxy commented Aug 10, 2023

The test failures were caused by _UIntBuffer creating another instance of itself in replaceSubrange combined with #65778, which forwarded from append(contentsOf:) back to replaceSubrange when there's contiguous storage available.

More detailed backtrace:

_UIntBuffer.replaceSubrange creates a new buffer from that forwards to the RangeReplaceableCollection.init<S: Sequence> method:

-> 212 	    let replacement1 = _UIntBuffer(replacement)

which then forwards to RangeReplaceableCollection.append(contentsOf:)

   409 	  public init<S: Sequence>(_ elements: S)
   410 	    where S.Element == Element {
   411 	    self.init()
-> 412 	    append(contentsOf: elements)
   413 	  }

which then forwards to withContiguousStorageIfAvailable -> replaceSubrange pre-revert.

   457 	    let done:Void? = newElements.withContiguousStorageIfAvailable {
-> 458 	      replaceSubrange(endIndex..<endIndex, with: $0)
   459 	    }

which led to infinite recursion, causing crashes in the tests above.

Since the change is now reverted in main (see #67842), tests should now pass / merging should be safe. PR is rebased.

@oxy
Copy link
Contributor Author

oxy commented Aug 10, 2023

@swift-ci please test

@oxy oxy marked this pull request as ready for review August 10, 2023 18:43
@oxy oxy requested review from glessard and lorentey August 10, 2023 18:43
@oxy
Copy link
Contributor Author

oxy commented Aug 11, 2023

@swift-ci please apple silicon benchmark

@oxy
Copy link
Contributor Author

oxy commented Aug 14, 2023

Performance (arm64): -O

Improvement OLD NEW DELTA RATIO
ConvertFloatingPoint.MockFloat64ToDouble 15.837 9.872 -37.7% 1.60x (?)
ConvertFloatingPoint.MockFloat64Exactly 5.148 3.672 -28.7% 1.40x (?)
ConvertFloatingPoint.MockFloat64Exactly2 5.438 3.959 -27.2% 1.37x (?)
ConvertFloatingPoint.MockFloat64ToInt64 391.5 333.5 -14.8% 1.17x (?)
ObserverForwarderStruct 204.615 179.231 -12.4% 1.14x (?)
DataAppendDataSmallToMedium 2069.412 1924.706 -7.0% 1.08x (?)

Code size: -O

Performance (arm64): -Osize

Regression OLD NEW DELTA RATIO
Set.isDisjoint.Seq.Int.Empty 43.652 49.92 +14.4% 0.87x (?)
Set.isDisjoint.Int.Empty 43.679 49.918 +14.3% 0.88x (?)
Set.isDisjoint.Box.Empty 45.217 51.458 +13.8% 0.88x (?)
Set.subtracting.Seq.Empty.Int 104.444 117.0 +12.0% 0.89x (?)
UTF16Decode.initFromCustom.cont 367.0 408.0 +11.2% 0.90x (?)
Set.subtracting.Seq.Int.Empty 111.111 121.0 +8.9% 0.92x (?)
Set.isStrictSubset.Empty.Int 79.538 85.826 +7.9% 0.93x (?)
 
Improvement OLD NEW DELTA RATIO
NaiveRRC.append.smallContiguousRepeated 79.222 72.609 -8.3% 1.09x (?)

Code size: -Osize

Performance (arm64): -Onone

Regression OLD NEW DELTA RATIO
ArrayAppendToGeneric 204.667 348.0 +70.0% 0.59x (?)
ArraySetElement 562.0 687.0 +22.2% 0.82x (?)
Set.filter.Int100.24k 62.575 69.714 +11.4% 0.90x
Set.filter.Int100.20k 52.585 58.476 +11.2% 0.90x
Set.filter.Int100.16k 42.6 47.231 +10.9% 0.90x
Set.filter.Int100.28k 75.036 83.067 +10.7% 0.90x (?)
String.replaceSubrange.ArrChar 25.414 27.903 +9.8% 0.91x
SIMDReduce.Int8x16.Cast 811.0 886.5 +9.3% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
ArrayAppendGenericStructs 842.308 496.429 -41.1% 1.70x (?)

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