[5.0] DataProtocol ContiguousCollection and new inline Data #21292

itaiferber · 2018-12-13T17:46:59Z

Cherry-pick of #20225
(cherry picked from commit d030354)

This is an implementation of DataProtocol and a new smaller and leaner struct Data. The major change here is that we now promise that all Data's are contiguous. In order to enforce this requisite we added a new protocol to define contiguous collections (aptly named ContiguousCollection). This protocol is a draft of the work in progress proposed API for the standard library. Additionally it offers a direct byte accessor if safe on sequence to unify the sequence initialization to allow for fast paths.

Data itself now is represented in 16 bytes(on 64 bit platforms). This allows it to be stored in existentials without needing to heap allocate. Additionally during this refactor to shrink the representation we were able to make similar wins to the small string optimization so that we can store Data's on the stack when under a platform limit. This makes small Data structures never have to heap allocate. For example: Data() has zero heap allocations and Data([0xd, 0xa]) wont heap allocate for the storage of the data either since small representations can be stored completely on the stack efficiently. Larger data or slices however will still have a reference type storing the backing allocation as they did before. Our hope is that this will reduce the traffic on malloc which will reduce memory fragmentation and be considerably more efficient for small buffers.

itaiferber · 2018-12-13T17:48:48Z

Please test with following pull request:
apple/swift-lldb#1138

@swift-ci Please test

itaiferber · 2018-12-13T17:55:12Z

I've gone ahead and incorporated the changes from #21290 as well here.

itaiferber · 2018-12-13T17:55:15Z

Please test with following pull request:
apple/swift-lldb#1138

@swift-ci Please test

swift-ci · 2018-12-13T17:55:43Z

Build failed
Swift Test OS X Platform
Git Sha - 856b5c8e773d70fb2573a0963c0808ab74ee3738

swift-ci · 2018-12-13T17:57:11Z

Build failed
Swift Test Linux Platform
Git Sha - 856b5c8e773d70fb2573a0963c0808ab74ee3738

(cherry picked from commit d030354)

itaiferber · 2018-12-13T21:01:05Z

Please test with following pull request:
apple/swift-lldb#1138

@swift-ci Please test

swift-ci · 2018-12-13T21:03:11Z

Build failed
Swift Test Linux Platform
Git Sha - 4bbf44d4d17927d5c95350670db45ed2b2761bbc

swift-ci · 2018-12-13T21:05:05Z

Build failed
Swift Test OS X Platform
Git Sha - 4bbf44d4d17927d5c95350670db45ed2b2761bbc

eeckstein · 2018-12-13T22:27:50Z

@itaiferber Did someone review this already?

itaiferber · 2018-12-13T22:29:46Z

@eeckstein Not in a formal capacity yet, no.

eeckstein · 2018-12-13T23:25:57Z

@airspeedswift Can you please review this?

airspeedswift · 2018-12-14T00:39:18Z

stdlib/public/SDK/Foundation/Data.swift

@@ -90,7 +78,7 @@ internal final class _DataStorage {
        }
    }

-    @usableFromInline
+    @inlinable


Are you sure this needs to be inlinable? It's not a generic function.

karwa · 2018-12-14T01:40:55Z

stdlib/public/SDK/Foundation/Data.swift

+    @inline(__always)
+    public init<S: Sequence>(_ elements: S) where S.Element == UInt8 {
+        // If the sequence is already contiguous, access the underlying raw memory directly.
+        if let contiguous = elements as? ContiguousBytes {


Just curious: why is this needed? Are there sequences of UInt8s which cannot implement withContiguousStorageIfAvailable but can conform to ContiguousBytes?

Yes: any sequence of UInt8's which does not represent typed memory cannot implement withContiguousStorageIfAvailable without handing over a copy, which defeats what we need to do here. Data and UnsafeRawBufferPointer are both examples of such sequences — both have an element type of UInt8, but represent raw memory, and thus cannot implement withContiguousStorageIfAvailable.

airspeedswift

Comments mostly are for future clean-up, though I would suggest inlinability could do with another pass to be certain it's justified everywhere. In some places the internals seem to be unnecessarily exposed in functions that don't seem critical to inline (apologies if experimentation has shown otherwise).

airspeedswift · 2018-12-14T00:50:07Z

stdlib/public/SDK/Foundation/Data.swift

-            block(UnsafeBufferPointer(start: bytePtr, count: effectiveLength - regionAdjustment), region.location + regionAdjustment - _offset, &stopv)
-            if stopv {
-                stop.pointee = true
+    @inlinable


This method is pretty huge, and not generic, so seems like there's not a strong reason to inline it. Did you see a noticeable perf difference from marking it inlined? Also it seems like it's the only reason some other functions need to be inlinable/usableFromInline.

airspeedswift · 2018-12-14T00:51:09Z

stdlib/public/SDK/Foundation/Data.swift

+        guard otherData.count > 0 else { return }
+        otherData.withUnsafeBytes {
+        append($0.baseAddress!, length: $0.count)
+    }


indenting nit

airspeedswift · 2018-12-14T00:54:22Z

stdlib/public/SDK/Foundation/Data.swift

+}
+
+@_fixed_layout
+public struct Data : ReferenceConvertible, Equatable, Hashable, RandomAccessCollection, MutableCollection, RangeReplaceableCollection, MutableDataProtocol, ContiguousBytes {


I'd recommend breaking this up some time into individual extensions for each conformance. We've found this helps readability of the code a fair bit in the std lib.

I agree — I didn't want to take on the task right now, but we should do this in the future.

airspeedswift · 2018-12-14T00:57:25Z

stdlib/public/SDK/Foundation/Data.swift

+        @inlinable
+        var count: Int {
+            get {
+                return numericCast(length)


No real need to use numericCast when you know both types in various places here. This should be Int(length). Being explicit about what you're converting to is clearer.

I'll change this too

airspeedswift · 2018-12-14T01:01:00Z

stdlib/public/SDK/Foundation/Data.swift

+                return numericCast(length)
+            }
+            set(newValue) {
+                precondition(newValue <= MemoryLayout<Buffer>.size)


This is internal, should it be an assert?

airspeedswift · 2018-12-14T01:39:49Z

stdlib/public/SDK/Foundation/Data.swift

+        }
+
+        @inlinable
+        var hashValue: Int {


Is the goal here having Data's hashValue match NSData's? If not, you should pass the value through Swift's hashing mechanism to ensure it gets randomly seeded.

airspeedswift · 2018-12-14T01:41:43Z

stdlib/public/SDK/Foundation/DataProtocol.swift

+//===--- DataProtocol Extensions ------------------------------------------===//
+
+extension DataProtocol {
+    public func firstRange<D: DataProtocol>(of data: D) -> Range<Index>? {


These are generic and entirely protocol-based helpers so should be inlinable.

airspeedswift · 2018-12-14T01:51:35Z

stdlib/public/SDK/Foundation/Data.swift

+                if range.upperBound == 0 {
+                    self = .empty
+                } else if InlineData.canStore(count: range.upperBound) {
+                    precondition(range.lowerBound <= endIndex, "index \(range.lowerBound) is out of bounds of \(startIndex)..<\(endIndex)")


Am I missing why the preconditions apply here but not below? Also could they be hoisted, they all seem the same.

airspeedswift · 2018-12-14T02:02:32Z

stdlib/public/SDK/Foundation/Data.swift

-#endif
-
-#if DEPLOYMENT_RUNTIME_SWIFT
-


similar comment to other hashValue, ideally this would use Swift's randomization

not sure why GitHub decided to make my comment here about a deleted line instead of the code on the rhs...

weird, it looks ok on the code tab

airspeedswift · 2018-12-14T02:17:59Z

stdlib/public/SDK/Foundation/Data.swift

+        var upperBound: Int { return range.upperBound }
+
+        @inlinable
+        var count: Int { return range.upperBound - range.lowerBound }


You should be able to replacerange.upperBound - range.lowerBound with range.count in a number of places.

Re: this and range.isEmpty — we found during testing that because range does not implement count itself (but instead gets the implementation from a default implementation on Collection), calculating this ourselves was orders of magnitude faster than calling range.count (dispatching through to the protocol implementation added a lot of overhead), and this was making a big difference in our unit tests.

It does appear that Range implements isEmpty directly, so we should be able to fix .isEmpty above.

Can you file a bug for the optimizer for that? We should try and ensure they're equivalent if possible. It might also be something that inlining too much is causing as the inliner might be giving up on a huge function that needs a specialized count the middle.

karwa · 2018-12-14T08:51:13Z

stdlib/public/SDK/Foundation/DataProtocol.swift

+    }
+
+    public func lastRange<D: DataProtocol>(of data: D) -> Range<Index>? {
+        return self.firstRange(of: data, in: self.startIndex ..< self.endIndex)


Should be lastRange

Good catch!

karwa · 2018-12-14T09:12:35Z

stdlib/public/SDK/Foundation/Data.swift

+                count = range.upperBound
+            }
+            Swift.withUnsafeMutableBytes(of: &bytes) { rawBuffer in
+                bzero(rawBuffer.baseAddress?.advanced(by: range.lowerBound), range.upperBound - range.lowerBound)


bzero is deprecated and doesn't exist on some platforms (e.g. windows). Use memset(3) instead.

http://pubs.opengroup.org/onlinepubs/009696699/functions/bzero.html

This implementation is only for Darwin platforms so bzero is fine, but I'll make this change so things are easier to copy over to swift-corelibs-foundation wholesale.

karwa · 2018-12-14T11:18:16Z

This is an interesting design and the performance improvements are welcome, but I'd nonetheless like to register my disappointment that this major change to the Swift API never went through swift-evolution.

itaiferber · 2018-12-14T17:12:11Z

@karwa The changes here are Foundation API, and the corelibs process is separate from swift-evolution. If we decide at any point to integrate this work into the standard library directly, we'll certainly go through swift-evolution, just like everyone else.

itaiferber · 2018-12-14T17:14:15Z

@airspeedswift Thanks for the review! I'm going to make some of the functional changes here that actually affect the implementation — as you mention, we can do the inlinability fixes in another pass very soon. We'd like to get the implementation landed first for testing and we have a little wiggle room to re-evaluate inlinability.

In general, we are aggressively inlining to give the compiler as much room as possible to make performance optimizations, which we've seen are largely necessary to make this work compelling. We can evaluate on a case-by-case basis if we see this having negative effects.

airspeedswift · 2018-12-14T17:34:16Z

In general, we are aggressively inlining to give the compiler as much room as possible to make performance optimizations

A word of warning – this can backfire. Marking things inlinable sometimes slows code down unexpectedly too because it limits what assumptions the compiler can make about internal uses of the inlinable function. It also bloats user binary size.

itaiferber · 2018-12-14T18:00:11Z

Please test with following pull request:
apple/swift-lldb#1138

@swift-ci Please test

swift-ci · 2018-12-14T18:02:13Z

Build failed
Swift Test Linux Platform
Git Sha - 482e87d

swift-ci · 2018-12-14T18:02:17Z

Build failed
Swift Test OS X Platform
Git Sha - 482e87d

itaiferber · 2018-12-14T22:28:34Z

@airspeedswift Do you have any further blocking concerns at this time? If not, we'll need to merge this and apple/swift-lldb#1138 in concert. (I can merge the lldb change but don't have permission to merge this.)

airspeedswift · 2018-12-14T23:39:48Z

I'm OK to merge this. Can you add some tests for the parts that had bugs that didn't get test failures in a follow-up PR?

palimondo · 2019-01-03T07:35:08Z

stdlib/public/SDK/Foundation/Data.swift

+
+                // ... and append the rest byte-wise.
+                while let element = iter.next() {
+                    Swift.withUnsafeBytes(of: element) {


This looks like a terrible performance pitfall to me... No benchmark currently covers this, but in my local test, this is 32x!!! slower than hitting the happy _copyContents path.

I see no API on _Representation to work with individual bytes, so this probably needs to be buffered locally here.

@palimondo Sorry for the delay in responding — just got back from vacation.
Just to clarify, do you mean by this comment that we should slurp all of the elements out of the iterator into an array, then append to _representation, or just that hitting this code path is slow? (Because the latter is somewhat expected — we've got a sequence here which is not ContiguousBytes, doesn't have an implementation of withContiguousStorageIfAvailable, and doesn't implement the _copyContents optimization, AKA, the worst-case scenario.)

The first. I don't think such slowdown is acceptable. Here's benchmark I got:

… BenchmarkInfo(name: "Data.append.Sequence.ExactCount", runFunction: { append($0*100, sequence: repeatElement(UInt8(0xA0), count: 809), to: medium) }, tags: d), BenchmarkInfo(name: "Data.append.Sequence.UnderestimatedCount", runFunction: { append($0*100, sequence: repeatElementSeq(809), to: medium) }, tags: d), … let repeatElementSeq = { count in return sequence(state: count) { (i: inout Int) -> UInt8? in defer { i = i &- 1 }; return i > 0 ? UInt8(0xA0) : nil } } @inline(never) func append<S: Sequence>(_ N: Int, sequence: S, to data: Data) where S.Element == UInt8 { for _ in 1...N { var copy = data copy.append(contentsOf: sequence) } }

...and just to clarify, we probably don't need to slurp all of the elements out of the iterator in one go, just amortize the costs of going over the UnsafeBufferPointer by slurping more elements in batches periodically append to the _representation. We could try reusing InlineData as the buffer, or test some moderately sized ContiguousArray…

@palimondo Sure, this is a reasonable change we can and should make. Are you interested in taking this on?

(We should make a similar change in append<S : Sequence>(_: S) as well.)

Not at the moment. I want to first finish my cleanup pass of the Swift Benchmark Suite, applying the legacyFactor.

Also, when I have played with this locally (I was println debugging Data implementation to see which branch get's exercised), I had trouble getting the build to work properly. Maybe it was just that most of the impl was inlined and the DataBenchmarks were't getting rebuilt? It was a mess.

So I cannot promise I could get back to this sooner than in two weeks...

@palimondo No worries, I'll take care of this, possibly in #21754

itaiferber requested a review from a team as a code owner December 13, 2018 17:46

itaiferber mentioned this pull request Dec 13, 2018

[5.0] Update Data summary provider with new ABI layout apple/swift-lldb#1138

Merged

itaiferber force-pushed the 5.0-inline_data_and_dataprotocol branch from 856b5c8 to 4bbf44d Compare December 13, 2018 17:54

Initial draft of DataProtocol ContiguousCollection and new inline Data

482e87d

(cherry picked from commit d030354)

itaiferber force-pushed the 5.0-inline_data_and_dataprotocol branch from 4bbf44d to 482e87d Compare December 13, 2018 21:00

airspeedswift reviewed Dec 14, 2018

View reviewed changes

karwa reviewed Dec 14, 2018

View reviewed changes

airspeedswift approved these changes Dec 14, 2018

View reviewed changes

karwa reviewed Dec 14, 2018

View reviewed changes

Itai Ferber added 2 commits December 14, 2018 09:57

Address review feedback

d7fcb87

Remove arm64_32 mention

615e182

airspeedswift merged commit 8ccc580 into apple:swift-5.0-branch Dec 14, 2018

karwa mentioned this pull request Dec 20, 2018

Import the new inline Data implementation and DataProtocol from the overlay apple/swift-corelibs-foundation#1813

Closed

palimondo reviewed Jan 3, 2019

View reviewed changes

This was referenced Jan 4, 2019

[benchmark] Janitor Duty: Datalore Legacy #21516

Merged

Data Inlinability Refinements #21754

Merged

ikesyo mentioned this pull request May 25, 2019

Fix DataProtocol.lastRange(of:) (and cherry-pick Data.swift changes from swift-5.0-branch) #25065

Merged

		#endif

		#if DEPLOYMENT_RUNTIME_SWIFT

[5.0] DataProtocol ContiguousCollection and new inline Data #21292

[5.0] DataProtocol ContiguousCollection and new inline Data #21292

Conversation

itaiferber commented Dec 13, 2018

itaiferber commented Dec 13, 2018

itaiferber commented Dec 13, 2018

itaiferber commented Dec 13, 2018

swift-ci commented Dec 13, 2018

swift-ci commented Dec 13, 2018

itaiferber commented Dec 13, 2018

swift-ci commented Dec 13, 2018

swift-ci commented Dec 13, 2018

eeckstein commented Dec 13, 2018

itaiferber commented Dec 13, 2018 • edited

eeckstein commented Dec 13, 2018

airspeedswift Dec 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airspeedswift left a 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

Choose a reason for hiding this comment

karwa commented Dec 14, 2018 • edited

itaiferber commented Dec 14, 2018

itaiferber commented Dec 14, 2018

airspeedswift commented Dec 14, 2018

itaiferber commented Dec 14, 2018

swift-ci commented Dec 14, 2018

swift-ci commented Dec 14, 2018

itaiferber commented Dec 14, 2018 • edited

airspeedswift commented Dec 14, 2018

palimondo Jan 3, 2019 • edited

Choose a reason for hiding this comment

itaiferber Jan 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palimondo Jan 8, 2019 • edited

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

itaiferber commented Dec 13, 2018 •

edited

airspeedswift Dec 14, 2018 •

edited

karwa commented Dec 14, 2018 •

edited

itaiferber commented Dec 14, 2018 •

edited

palimondo Jan 3, 2019 •

edited

itaiferber Jan 7, 2019 •

edited

palimondo Jan 8, 2019 •

edited