[stdlib] [WIP] Add UnsafeRawBufferPointer.initialize(as:from:) #5718

airspeedswift · 2016-11-11T03:01:26Z

Along similar lines to the changes in this PR.

Deprecates UnsafeRawPointer.initializeMemory(as:from:) in favor of UnsafeRawBufferPointer.initializeMemory(as:from:), which performs the same task but 1) takes a sequence rather than a collection, 2) has the precondition that the buffer be at least as big as to accommodate underestimatedCount, and 3) returns an iterator with the unwritten elements, as well as an index one past the last byte written.

This is because calling UnsafeRawPointer.initializeMemory(as:from:) is unsafe to use in a generic context, as there is no way to guarantee that the appended collection's count isn't an overestimate.

airspeedswift · 2016-11-11T03:01:38Z

@swift-ci Please test

airspeedswift · 2016-11-11T03:03:49Z

Hi @atrick – as requested on your review of the other change.

Thinking about it, I think the check of underestimatedCount needs to be an assert rather than a precondition, as otherwise it would end up walking a lazy collection twice in release mode which is quite inefficient.

swift-ci · 2016-11-11T03:46:56Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 3cb644c
Test requested by - @airspeedswift

Gankra · 2016-11-11T03:10:42Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+    // TODO: Optimize where `C` is a `ContiguousArrayBuffer`.
+    public func initializeMemory<S: Sequence>(
+      as: S.Iterator.Element.Type, from source: S
+    ) -> (S.Iterator, Index) {


When is using labels in return tuples considered correct? When the types leave it ambiguous? Not knowing our conventions here, (unwritten: S.Iterator, bytesWritten: Index) makes the signature a lot more clear.

yes, you're right, that would be clearer. Though it's an index, so it'd be writtenUpTo:

bytesWritten is clearer to me than writtenUpTo. They're of course equivalent, but bytesWritten is totally ambiguous while writtenUpTo leaves lingering questions of inclusivity and what the unit is (elements or bytes?). It's clear in the docs, but won't be clear to someone reading some code that happens to use this function.

But I'll defer to someone who knows the API guidelines better.

The type is an Index not a byte count. Maybe this is splitting hairs in this case because the index on a buffer is also the byte count... but it's not good to rely on that, because sometimes slices of collections end up not having zero-based indices.

Gankra · 2016-11-11T03:12:14Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+      let underestimate = source.underestimatedCount
+      guard let base = self.baseAddress else {
+        _precondition(underestimate == 0, "no memory available to initialize from source")
+        return (source.makeIterator(),startIndex)


style: space after comma

Gankra · 2016-11-11T03:30:02Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+
+      for p in stride(from: base, 
+        // only advance to as far as the last element that will fit
+        to: base + count - (count % MemoryLayout<S.Iterator.Element>.stride), 


Isn't this equivalent to base + count? This is basically rounding count down to the nearest multiple of stride, but since we're incrementing by stride we'll always stop at a multiple of stride. It doesn't randomly include to if that's not a multiple.

But you need to stride over the starting points for the elements up to the last starting point that will fit another element. to: base+count would stride up to the starting point of the last element that won't fit i.e. if it were 10 byte buffer and 8 byte elements, the last stride(from: 0, to: 10, by: 8) returns 8 but you can't write another element there. I've no doubt this could be expressed more clearly than what I've written here tho.

Ah right. base + count - stride + 1? (really just want to avoid modulo)

quick check:

(count: 10, stride: 8 ) = 0..<3 = [0] (count: 10, stride: 10) = 0..<1 = [0] (count: 10, stride: 5 ) = 0..<6 = [0, 5] (count: 11, stride: 6 ) = 0..<6 = [0] (count: 10, stride: 4 ) = 0..<7 = [0, 4]

Gankra · 2016-11-11T03:35:15Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+        guard let x = it.next() else { break }
+        p.initializeMemory(as: S.Iterator.Element.self, to: x)
+        formIndex(&idx, offsetBy: MemoryLayout<S.Iterator.Element>.stride)
+      }


Optimizing this kind of loop well (so that it lowers to a memcpy when it should) has historically been a really big struggle. Slightly different because it reallocates, but this is the best version the Rust devs have been able to produce, if you're interested in aggressive micro-optimizations: https://doc.rust-lang.org/src/collections/up/src/libcollections/vec.rs.html#1550-1570

If we're serious about it, it might be worth even having an IRGen test verifying that e.g. passing Array<Int8> to this lowers to memcpy. (not a blocker, just some idle thoughts)

Gankra · 2016-11-11T03:35:20Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+        formIndex(&idx, offsetBy: MemoryLayout<S.Iterator.Element>.stride)
+      }
+
+      return (it,idx)


Style: space after comma.

Gankra · 2016-11-11T03:39:45Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+        // the spare capacity of an Array buffer
+        guard let x = it.next() else { break }
+        p.initializeMemory(as: S.Iterator.Element.self, to: x)
+        formIndex(&idx, offsetBy: MemoryLayout<S.Iterator.Element>.stride)


Since you use stride 3 times, I'd cache it into a variable, if only for code cleanliness.

A variable I'll need to call something other than stride :(

Oh right, stride function. Gross. elem(ent)Stride?

Gankra · 2016-11-11T03:40:28Z

stdlib/public/core/UnsafeRawPointer.swift.gyb

@@ -294,7 +294,9 @@ public struct Unsafe${Mutable}RawPointer : Strideable, Hashable, _Pointer {
  /// - Postcondition: The `T` values at `self..<self + source.count *
  ///   MemoryLayout<T>.stride` are initialized.
  ///
-  /// TODO: Optimize where `C` is a `ContiguousArrayBuffer`.
+  // TODO: Optimize where `C` is a `ContiguousArrayBuffer`.


Presumably if this shouldn't be used, optimizing it isn't interesting?

fair point!

Gankra · 2016-11-11T03:42:32Z

test/stdlib/UnsafeRawBufferPointer.swift

+  defer { buffer.deallocate() }
+  let source: [Int64] = [5,4,3,2,1]
+  expectCrashLater()
+  var (it,idx) = buffer.initializeMemory(as: Int64.self, from: source)


Style: space after comma (and in the array, and in the above function)

Oh: is expectCrashLater() capable of distinguishing an explicit assertion (safe) from a segfault (UB)?

Gankra · 2016-11-11T03:42:34Z

test/stdlib/UnsafeRawBufferPointer.swift

+  var (it,idx) = buffer.initializeMemory(as: Int64.self, from: source)
+}
+
+
 // Directly test the byte Sequence produced by withUnsafeBytes.


You should have tests verifying both of the null pointer behaviours.

You should have a test verifying the case where the iterator is exhausted with slack space. (the final sequence is empty)

Possibly a test that verifies that the "exactly enough space" case works (off by ones will getcha!).

Gankra · 2016-11-11T03:48:16Z

test/stdlib/UnsafeRawBufferPointer.swift

+  ([5,4,3] as [Int64]).withUnsafeBytes {
+    expectEqualSequence($0,buffer[0..<idx])
+  }
+}


Is this testing the situation where the underEstimate is actually too small? (Not sure how accurate stride is)

Yes, but that's allowed (it is an underestimate after all). Your point about testing exactly-rightly-sized buffers too is taken tho.

natecook1000 · 2016-11-11T17:53:16Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+    ///   MemoryLayout<T>.stride` is bound to type `T`.
+    ///
+    /// - Postcondition: The `T` values at `self..<self + source.count *
+    ///   MemoryLayout<T>.stride` are initialized.


This postcondition isn't accurate any more, since count isn't available on a sequence and the buffer only initializes items until it runs out of room—hard to figure out how to write this concisely.

natecook1000 · 2016-11-11T17:53:20Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

@@ -348,6 +348,56 @@ public struct Unsafe${Mutable}RawBufferPointer
    return 0
  }

+  %  if mutable:
+    /// Initializes memory in the buffer with the elements of


I know it's crazy, but GYB control blocks shouldn't indent the Swift code they wrap.

airspeedswift · 2016-11-12T19:49:30Z

@swift-ci Please test

swift-ci · 2016-11-12T20:37:32Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 3cb644c
Test requested by - @airspeedswift

Changes seem good.

Gankra · 2016-11-14T22:48:58Z

stdlib/public/core/UnsafeRawBufferPointer.swift.gyb

+  /// - Postcondition: The memory at `self[startIndex..<returned index]                 
+  ///   is bound to type `T`.
+  ///
+  /// - Postcondition: The `T` values at `self[startIndex..<returned index]`


Is it fair game to refer to initializedUpTo here?

atrick

Otherwise looks great!

atrick · 2016-11-15T01:34:03Z

test/stdlib/UnsafeRawBufferPointer.swift

@@ -75,6 +75,59 @@ UnsafeRawBufferPointerTestSuite.test("initFromArray") {
  expectEqual(array2, array1)
 }

+UnsafeRawBufferPointerTestSuite.test("initializeMemory(as:from:).underflow") {


I'm confused by the underflow terminology here. This sequence still overflows the buffer right? Isn't it an overflow with underestimated count?

I don't see a test case for actual underflow!

airspeedswift · 2017-01-04T17:57:52Z

Closing in order to raise a fresh PR combining this and the change to UnsafeMutablePointer.

Add UnsafeRawBufferPointer.initialize(as:from:)

3cb644c

airspeedswift changed the title ~~[stdlib] Add UnsafeRawBufferPointer.initialize(as:from:)~~ [stdlib] [WIP] Add UnsafeRawBufferPointer.initialize(as:from:) Nov 11, 2016

Gankra previously requested changes Nov 11, 2016

View reviewed changes

natecook1000 reviewed Nov 11, 2016

View reviewed changes

accounting for review feedback

cd4c4c0

Gankra reviewed Nov 14, 2016

View reviewed changes

atrick reviewed Nov 15, 2016

View reviewed changes

airspeedswift closed this Jan 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] [WIP] Add UnsafeRawBufferPointer.initialize(as:from:) #5718

[stdlib] [WIP] Add UnsafeRawBufferPointer.initialize(as:from:) #5718

airspeedswift commented Nov 11, 2016 •

edited

airspeedswift commented Nov 11, 2016

airspeedswift commented Nov 11, 2016

swift-ci commented Nov 11, 2016

Gankra Nov 11, 2016

airspeedswift Nov 11, 2016

Gankra Nov 11, 2016

airspeedswift Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

airspeedswift Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

airspeedswift Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

airspeedswift Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

Gankra Nov 11, 2016

airspeedswift Nov 11, 2016

natecook1000 Nov 11, 2016 •

edited

natecook1000 Nov 11, 2016

airspeedswift commented Nov 12, 2016

swift-ci commented Nov 12, 2016

Gankra Nov 14, 2016

atrick left a comment

atrick Nov 15, 2016

airspeedswift commented Jan 4, 2017

[stdlib] [WIP] Add UnsafeRawBufferPointer.initialize(as:from:) #5718

[stdlib] [WIP] Add UnsafeRawBufferPointer.initialize(as:from:) #5718

Conversation

airspeedswift commented Nov 11, 2016 • edited

airspeedswift commented Nov 11, 2016

airspeedswift commented Nov 11, 2016

swift-ci commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natecook1000 Nov 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airspeedswift commented Nov 12, 2016

swift-ci commented Nov 12, 2016

Choose a reason for hiding this comment

atrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airspeedswift commented Jan 4, 2017

airspeedswift commented Nov 11, 2016 •

edited

natecook1000 Nov 11, 2016 •

edited