[stdlib] non-mutating range subscript setter on UnsafeMutableBufferPointer #12504

glessard · 2017-10-19T04:35:27Z

In an apparent oversight (?), the setter for UnsafeMutableBufferPointer subscript(bounds: Range<Int>) isn't marked as nonmutating. Given that UnsafeMutableRawBufferPointer's equivalent is nonmutating, here is a non-mutating implementation.

I believe this shouldn't require swift-evolution involvement, but if this gets merged some new "un-mutated var" warnings may appear.

SR-3782 mentions this. Someone mentioned working on it months ago, but there did not appear to be a pull request.

airspeedswift

Thanks for picking this up! Couple of comments inline. This also needs some tests confirming it's working as expected against immutable buffer variables (and tests in general, if that isn't already isn't already covered by existing unit tests).

airspeedswift · 2017-10-26T14:29:43Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

      // FIXME: swift-3-indexing-model: tests.
-      _writeBackMutableSlice(&self, bounds: bounds, slice: newValue)
+      if newValue.count > 0 {


This should be !newValue.isEmpty. I realize it's not a big deal in this case since we're talking about integer-indexed collections here but we should never use count to check for emptiness.

Feel free to change UnsafeMutableRawBufferPointer implementation as well.

airspeedswift · 2017-10-26T14:57:39Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+      if newValue.count > 0 {
+        var sliceIndex = newValue.startIndex
+        var index = bounds.lowerBound
+        while index != bounds.upperBound && sliceIndex != newValue.endIndex {


Given we know these are two unsafe buffers (unlike _writeBackMutableSlice , which is generic) we ought to be able to just do a straight memory copy, once we've checked the count is the same. Something like...

baseAddress!.initialize(from: newValue.base.baseAddress!, count: bounds.count)

The only thing that would worry me about if this is correct is the possibility of overlapping assignment from self...

UnsafeMutablePointer.assign supports overlapping ranges. That's the thing to use.

Yes, exactly but use assignment.
baseAddress!.assign(from: newValue.base.baseAddress!, count: bounds.count)

That does the correct forward or backward copy depending on the relative pointer positions.

airspeedswift · 2017-10-26T14:58:33Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

      _debugPrecondition(bounds.lowerBound >= startIndex)
      _debugPrecondition(bounds.upperBound <= endIndex)
+      _debugPrecondition(bounds.count == newValue.count)


I think this one should be a full _precondition (similar to the one found in _writeBackMutableSlice).

The analog line in UnsafeRawBufferPointer uses a _debugPrecondition: https://github.com/apple/swift/blob/ffd34239a1b6f046346769a55ae8e717f64e13eb/stdlib/public/core/UnsafeRawBufferPointer.swift.gyb#L468
Is there a reason to make it stronger in the typed buffers, or should it be a _precondition in both places?

The reasoning behind _debugPrecondition is that this is already an Unsafe type and being at the the lowest level of abstraction will be used in performance critical code.

atrick · 2017-10-26T15:52:06Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  ///   the range must be valid indices of the buffer.
+  ///
+%if Mutable:
+  /// - Complexity: O(1) for reading, O(N) for writing


I see where the complexity bounds comes from, but I think it's misleading. It subtly reflects the fact that "reading" is deferred until the slice is used. And N will likely be mistaken for the buffer count. Since there's nothing magic or unexpected happening here, why talk about complexity? @airspeedswift, is this something we need to document?

This is the same complexity that's documented at the protocol level, so we can remove it here. We only document on types when there's a deviation from the expectation (like Set.count).

natecook1000 · 2017-10-26T16:05:10Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+  ///   the range must be valid indices of the buffer.
+  ///
+%if Mutable:
+  /// - Complexity: O(1) for reading, O(N) for writing


This is the same complexity that's documented at the protocol level, so we can remove it here. We only document on types when there's a deviation from the expectation (like Set.count).

natecook1000 · 2017-10-26T16:10:56Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

+%if Mutable:
+  ///     var streets = ["Adams", "Bryant", "Channing", "Douglas", "Evarts"]
+  ///     streets.withUnsafeMutableBufferPointer { buffer in
+  ///       let streetSlice = buffer[2..<buffer.endIndex]


Thanks for adapting these docs! Just to keep everyone on their toes, we use four-space indentation in documentation examples, instead of the two used in code.

oh? I hadn't noticed!

glessard · 2017-10-26T16:43:16Z

Regarding tests: I had assumed that when the tests passed it had been covered.
However, this is covered in validation-tests, but not in the regular tests. UnsafeRawBufferPointer has a test suite under tests/stdlib; it could be adapted for UnsafeBufferPointer, or this would remain covered during validation only. What's the recommendation?

atrick · 2017-10-26T16:53:05Z

UnsafeRawBufferPointer has a separate set of tests that cover behavior specific to that type, like non-Collection APIs and debug bounds checks. Unlike the validation test, they are not meant to be exhaustive. UnsafeBufferPointer could probably have the same thing if you think it's useful. I don't have a strong opinion.

airspeedswift · 2017-10-26T17:04:24Z

I just realized, the setter should account for the fact that a mutation of self doesn't need to actually be written back, because reference semantics mean it's already happened.

i.e.:

someUnsafeBuffer[subrange].sort()

will generate first a call to get, then sort the slice, then write the sorted slice back via set. But when the type has reference semantics, the write-back is unnecessary, because the mutation has already happened on the underlying data, via the slice. So the non mutating set should check the identity of the buffer pointer to check for this scenario.

glessard · 2017-10-26T17:21:48Z

Is the pointer identity not checked by the builtin function called by assign(from:count:)?

glessard · 2017-10-27T05:07:06Z

stdlib/public/core/UnsafeBufferPointer.swift.gyb

-          count: newValue.count)
+        let source = newValue.base._position! + newValue.startIndex
+        let destination = _position! + bounds.lowerBound
+        if source != destination {


If it's appropriate to do this check, is there a reason not to do it directly as an else if in assign?
https://github.com/apple/swift/blob/e4236405279f42345e12eebbc023a3409a034b7a/stdlib/public/core/UnsafePointer.swift.gyb#L562-L564

or even all the way down in array_copy_operation?
https://github.com/apple/swift/blob/e4236405279f42345e12eebbc023a3409a034b7a/stdlib/public/runtime/Array.cpp#L86-L87

glessard · 2017-10-27T15:46:03Z

UnsafeMutableBufferPointer and UnsafeMutableRawBufferPointer both need a non-mutating public subscript<R: RangeExpression>(r: R) (and perhaps public subscript(x: UnboundedRange)). Is that an evolution topic?

atrick · 2017-10-27T21:25:46Z

Is the pointer identity not checked by the builtin function called by assign(from:count:)?

Possibly somewhere in the runtime or libc. The point is, we don't even want to make that series of runtime calls. Checking in the else case of assign seems reasonable to me. Maybe that should also be in the doc comment since we want to make that assumption when called from UnsafeMutableBufferPointer.

atrick · 2017-10-28T06:20:23Z

👍

airspeedswift · 2017-10-28T18:40:14Z

@swift-ci please test

swift-ci · 2017-10-28T18:41:30Z

Build failed
Swift Test Linux Platform
Git Sha - b4bc6d3a2e0cbbb0ad91ab3ce46a002768fd16f4

swift-ci · 2017-10-28T18:44:33Z

Build failed
Swift Test OS X Platform
Git Sha - b4bc6d3a2e0cbbb0ad91ab3ce46a002768fd16f4

glessard · 2017-10-28T20:59:50Z

The CollectionType validation test is expecting a particular crash message in withUnsafeMutableBufferPointer for the debug precondition
_debugPrecondition(bounds.count == newValue.count)
I'm not sure whether to just copy/paste the message or change the expectation...

atrick · 2017-10-30T15:26:30Z

The addition of debug messages seems fine to me.

glessard · 2017-10-30T17:24:41Z

Upon looking at it again, the validation tests that failed are called "subscript(_: Range)/defaultImplementation". They currently work by modifying an array through withUnsafeMutableBufferPointer in order to get a non-range-replaceable MutableCollection, but they're not testing the default implementation (_writeBackMutableSlice) anymore. I'm looking for a different MutableCollection they could call through.

glessard · 2017-11-09T19:54:12Z

Squashed commits. Do we want fewer still?

atrick · 2017-11-09T20:36:30Z

Either way, 1 commit or 4 commits, is fine with me.

glessard · 2017-11-09T22:48:49Z

OK then; to my eye it seems ready to merge.

airspeedswift · 2017-11-16T23:50:14Z

@swift-ci please test

airspeedswift · 2017-12-02T00:42:53Z

@swift-ci please test

swift-ci · 2017-12-02T00:43:10Z

Build failed
Swift Test Linux Platform
Git Sha - b0f66f835c08d8e009973ac7937d6235779c4c94

airspeedswift · 2017-12-02T00:43:14Z

@swift-ci please test source compatibility

swift-ci · 2017-12-02T00:43:15Z

Build failed
Swift Test OS X Platform
Git Sha - b0f66f835c08d8e009973ac7937d6235779c4c94

airspeedswift · 2017-12-02T00:43:22Z

@swift-ci please smoke test compiler performance

swift-ci · 2017-12-02T01:18:17Z

Build comment file:

Summary for master smoketest

No regressions above thresholds

Debug
- debug brief
- debug detailed
Release
- release brief
- release detailed

Debug

debug brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	23,285,028	23,285,020	-8	-0.0%
time.swift-driver.wall	30.9s	30.8s	-77.2ms	-0.25%

debug detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (23)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	47,780	47,780	0	0.0%
AST.NumLoadedModules	6,369	6,369	0	0.0%
AST.NumTotalClangImportedEntities	133,186	133,186	0	0.0%
AST.NumUsedConformances	2,713	2,713	0	0.0%
IRModule.NumIRBasicBlocks	65,495	65,495	0	0.0%
IRModule.NumIRFunctions	32,935	32,935	0	0.0%
IRModule.NumIRGlobals	36,442	36,442	0	0.0%
IRModule.NumIRInsts	702,158	702,158	0	0.0%
IRModule.NumIRValueSymbols	59,725	59,725	0	0.0%
LLVM.NumLLVMBytesOutput	23,285,028	23,285,020	-8	-0.0%
SILModule.NumSILGenFunctions	14,638	14,638	0	0.0%
SILModule.NumSILOptFunctions	25,709	25,707	-2	-0.01%
Sema.NumConformancesDeserialized	114,967	114,967	0	0.0%
Sema.NumConstraintScopes	416,017	416,017	0	0.0%
Sema.NumDeclsDeserialized	906,521	906,521	0	0.0%
Sema.NumDeclsValidated	28,449	28,449	0	0.0%
Sema.NumFunctionsTypechecked	27,343	27,343	0	0.0%
Sema.NumGenericSignatureBuilders	36,829	36,829	0	0.0%
Sema.NumLazyGenericEnvironments	185,054	185,054	0	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	18,665	18,665	0	0.0%
Sema.NumLazyIterableDeclContexts	152,672	152,672	0	0.0%
Sema.NumTypesDeserialized	967,524	967,486	-38	-0.0%
Sema.NumTypesValidated	97,068	97,068	0	0.0%

Release

release brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	28,803,372	28,803,372	0	0.0%
time.swift-driver.wall	66.8s	66.8s	-24.5ms	-0.04%

release detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (23)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	7,421	7,421	0	0.0%
AST.NumLoadedModules	227	227	0	0.0%
AST.NumTotalClangImportedEntities	22,601	22,601	0	0.0%
AST.NumUsedConformances	2,719	2,719	0	0.0%
IRModule.NumIRBasicBlocks	61,475	61,475	0	0.0%
IRModule.NumIRFunctions	26,597	26,597	0	0.0%
IRModule.NumIRGlobals	34,113	34,113	0	0.0%
IRModule.NumIRInsts	581,886	581,886	0	0.0%
IRModule.NumIRValueSymbols	54,292	54,292	0	0.0%
LLVM.NumLLVMBytesOutput	28,803,372	28,803,372	0	0.0%
SILModule.NumSILGenFunctions	10,842	10,842	0	0.0%
SILModule.NumSILOptFunctions	19,650	19,655	5	0.03%
Sema.NumConformancesDeserialized	68,558	68,558	0	0.0%
Sema.NumConstraintScopes	395,763	395,763	0	0.0%
Sema.NumDeclsDeserialized	133,769	133,769	0	0.0%
Sema.NumDeclsValidated	17,632	17,632	0	0.0%
Sema.NumFunctionsTypechecked	7,471	7,471	0	0.0%
Sema.NumGenericSignatureBuilders	4,540	4,540	0	0.0%
Sema.NumLazyGenericEnvironments	23,679	23,679	0	0.0%
Sema.NumLazyGenericEnvironmentsLoaded	2,660	2,660	0	0.0%
Sema.NumLazyIterableDeclContexts	12,645	12,645	0	0.0%
Sema.NumTypesDeserialized	157,620	157,601	-19	-0.01%
Sema.NumTypesValidated	31,082	31,082	0	0.0%

glessard · 2017-12-08T21:21:55Z

validation-test/stdlib/CollectionType.swift.gyb

@@ -580,21 +580,17 @@ CollectionTypeTests.test("subscript(_: Range<Index>)/writeback") {
 CollectionTypeTests.test("subscript(_: Range<Index>)/defaultImplementation/sliceTooLarge")
  .crashOutputMatches("Cannot replace a slice of a MutableCollection with a slice of a larger size")
  .code {
-  var x = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
+  var x = Slice(base: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], bounds: 0..<10)


Changed from MutableSlice to Slice. Conditional Conformances are nice.

lorentey · 2017-12-13T16:48:45Z

@swift-ci please test

glessard · 2018-03-06T01:26:35Z

@atrick Sorry to bother, but is this PR waiting on something else I'm not aware of?

atrick · 2018-03-06T01:59:19Z

@glessard It looks like I was the first to give this a thumbs up but was waiting for @airspeedswift review. @lorentey, you last ran a successful test here, do you want to retest and merge?

lorentey · 2018-03-06T12:39:13Z

@swift-ci please test and merge

glessard · 2018-03-06T14:48:41Z

Thanks

glessard force-pushed the umbp-nonmutating-setter branch 3 times, most recently from c6354c9 to 479c94b Compare October 19, 2017 11:20

glessard changed the title ~~a non-mutating range subscript setter on UnsafeMutableBufferPointer~~ [stdlib] non-mutating range subscript setter on UnsafeMutableBufferPointer Oct 19, 2017

glessard force-pushed the umbp-nonmutating-setter branch 2 times, most recently from 4223418 to d72b1ac Compare October 20, 2017 17:34

airspeedswift requested changes Oct 26, 2017

View reviewed changes

airspeedswift requested a review from atrick October 26, 2017 15:01

atrick reviewed Oct 26, 2017

View reviewed changes

natecook1000 reviewed Oct 26, 2017

View reviewed changes

glessard mentioned this pull request Oct 26, 2017

[stdlib] use Collection.isEmpty rather than Collection.count #12638

Merged

glessard commented Oct 27, 2017

View reviewed changes

glessard force-pushed the umbp-nonmutating-setter branch from 7db7a1c to a9daabd Compare October 27, 2017 22:53

airspeedswift approved these changes Oct 28, 2017

View reviewed changes

glessard force-pushed the umbp-nonmutating-setter branch from e8869e2 to 26b3de1 Compare October 31, 2017 04:57

glessard force-pushed the umbp-nonmutating-setter branch 2 times, most recently from 8f7daf7 to 24ab86c Compare November 9, 2017 19:49

glessard force-pushed the umbp-nonmutating-setter branch from 24ab86c to b0f66f8 Compare November 9, 2017 19:51

glessard force-pushed the umbp-nonmutating-setter branch 2 times, most recently from 93912aa to e2b8520 Compare November 23, 2017 17:51

glessard mentioned this pull request Nov 26, 2017

nonmutating swapAt implementations for UnsafeMutable(Raw)BufferPointer #13071

Merged

glessard mentioned this pull request Dec 8, 2017

[stdlib] add more debug-mode range checking to UnsafeBufferPointer #13340

Merged

glessard added 4 commits December 8, 2017 14:14

a non-mutating range subscript setter for UnsafeMutableBufferPointer

894585f

skip assignment when source and destination are the same address

9eeb6a0

test non-mutating and overlap behaviour of slice assignments

4683eff

make "subscript/defaultImplementation" test the default implementation

0fb0d51

glessard force-pushed the umbp-nonmutating-setter branch from e2b8520 to 0fb0d51 Compare December 8, 2017 21:18

glessard commented Dec 8, 2017

View reviewed changes

swift-ci merged commit abbaa47 into swiftlang:master Mar 6, 2018

glessard deleted the umbp-nonmutating-setter branch March 7, 2018 19:14

ikesyo mentioned this pull request Mar 20, 2018

[gardening][(JSON|Plist)Encoder] Prefer !isEmpty over count > 0 #15335

Merged

glessard mentioned this pull request Oct 25, 2017

[SR-3782] UnsafeMutable[Raw]BufferPointer should have nonmutating subscript setters #46367

Closed

[stdlib] non-mutating range subscript setter on UnsafeMutableBufferPointer #12504

[stdlib] non-mutating range subscript setter on UnsafeMutableBufferPointer #12504

Uh oh!

Conversation

glessard commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

airspeedswift 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!

airspeedswift Oct 26, 2017 • 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!

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!

glessard commented Oct 26, 2017

Uh oh!

atrick commented Oct 26, 2017

Uh oh!

airspeedswift commented Oct 26, 2017

Uh oh!

glessard commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glessard commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrick commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrick commented Oct 28, 2017

Uh oh!

airspeedswift commented Oct 28, 2017

Uh oh!

swift-ci commented Oct 28, 2017

Uh oh!

swift-ci commented Oct 28, 2017

Uh oh!

glessard commented Oct 28, 2017

Uh oh!

atrick commented Oct 30, 2017

Uh oh!

glessard commented Oct 30, 2017

Uh oh!

glessard commented Nov 9, 2017

Uh oh!

atrick commented Nov 9, 2017

Uh oh!

glessard commented Nov 9, 2017

Uh oh!

airspeedswift commented Nov 16, 2017

Uh oh!

airspeedswift commented Dec 2, 2017

Uh oh!

glessard commented Oct 19, 2017 •

edited

Loading

airspeedswift Oct 26, 2017 •

edited

Loading

glessard commented Oct 27, 2017 •

edited

Loading

atrick commented Oct 27, 2017 •

edited

Loading