[Swift] Provide access to underlying pointer for Scalars and NativeStructs #8481
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
c++
codegen
wtholliday
reviewed
Jan 13, 2025
tests/swift/tests/Tests/FlatBuffers.Test.SwiftTests/ByteBufferTests.swift
Show resolved
Hide resolved
mustiikhalil
force-pushed
the
mmk/provide-access-underlying-pointer
branch
from
8ecad3f to
13150ef
Compare
mustiikhalil
force-pushed
the
mmk/provide-access-underlying-pointer
branch
from
13150ef to
5953d40
Compare
…teBuffer Adds a way to store external data and use them direclty within them ByteBuffer instead of copying memory into the internal memory of the ByteBuffer Removed unowned from the initializer Fixes a bug where array's of bytes are still being copied Adds benchmark for initializing memory into a bytebuffer Address a bug where the bytebuffer starts a memory leak Replaces assumingMemoryBinding with bindMemory which is safer
mustiikhalil
force-pushed
the
mmk/provide-access-underlying-pointer
branch
2 times, most recently
from
07fab73 to
b8aca98
Compare
Removed the use of Array to create a string Updates docs suppress compilation warnings by replacing var with let
mustiikhalil
force-pushed
the
mmk/provide-access-underlying-pointer
branch
from
b8aca98 to
7b92b51
Compare
wtholliday
reviewed
Jan 14, 2025
| @@ -122,6 +197,7 @@ public struct MyGame_Sample_Monster: FlatBufferObject, Verifiable { | |||
| public var inventoryCount: Int32 { let o = _accessor.offset(VTOFFSET.inventory.v); return o == 0 ? 0 : _accessor.vector(count: o) } | |||
| public func inventory(at index: Int32) -> UInt8 { let o = _accessor.offset(VTOFFSET.inventory.v); return o == 0 ? 0 : _accessor.directRead(of: UInt8.self, offset: _accessor.vector(at: o) + index * 1) } | |||
| public var inventory: [UInt8] { return _accessor.getVector(at: VTOFFSET.inventory.v) ?? [] } | |||
| public var inventoryPointer: UnsafeBufferPointer<UInt8>? { return _accessor.getBufferPointer(at: VTOFFSET.inventory.v) } | |||
There was a problem hiding this comment.
I think it might be better to have a withInventoryPointer style API which ensures the pointer is valid for the duration of the closure. Seems weird to be able to pass in a Data which requires that sort of API, but then evade it later.
mustiikhalil
commented
Jan 14, 2025
Comment on lines
+69
to
+81
| case .data(let data): | ||
| (memory, capacity) = data.withUnsafeBytes { | ||
| (UnsafeMutableRawPointer(mutating: $0.baseAddress!), $0.count) | ||
| } | ||
| case .bytes(let bytes): | ||
| (memory, capacity) = bytes.withUnsafeBytes { | ||
| (UnsafeMutableRawPointer(mutating: $0.baseAddress!), $0.count) | ||
| } | ||
| #endif | ||
| case .array(let array): | ||
| (memory, capacity) = array.withUnsafeBytes { | ||
| (UnsafeMutableRawPointer(mutating: $0.baseAddress!), $0.count) | ||
| } |
There was a problem hiding this comment.
@wtholliday I think that's fine because we technically return the pointer from the withUnsafeBytes here. But its doable for sure to make it extra safe
There was a problem hiding this comment.
Well, calling withUnsafeBytes and returning the pointer is bad practice AFAIK.
There was a problem hiding this comment.
then assigning it similar to what we had earlier is also wrong :/ cause their ways we are escaping the closure and returning the pointer.
// number 1
array.withUnsafeBytes {
self.memory = UnsafeMutableRawPointer(mutating: $0.baseAddress!)
self.capacity = $0.count
}
// number 2
(memory, capacity) = array.withUnsafeBytes {
(UnsafeMutableRawPointer(mutating: $0.baseAddress!), $0.count)
}There was a problem hiding this comment.
@mzaks a bit of feedback is appreciated here, it seems that we are stuck.
1- We either copy the data
2- We slow down the library using closure, just to access swift internal data.
There was a problem hiding this comment.
@wtholliday this is the difference between both read double benchmarks, current implementation:
Reading Doubles
╒═══════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│ Metric │ p0 │ p25 │ p50 │ p75 │ p90 │ p99 │ p100 │ Samples │
╞═══════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│ Malloc (total) * │ 0 │ 0 │ 0 │ 0 │ 0 │ 0 │ 0 │ 1528 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Memory (resident peak) (K) │ 8126 │ 8471 │ 8471 │ 8471 │ 8471 │ 8471 │ 8471 │ 1528 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Releases * │ 0 │ 0 │ 0 │ 0 │ 0 │ 0 │ 0 │ 1528 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (total CPU) (ns) * │ 2 │ 2 │ 2 │ 2 │ 2 │ 2 │ 2 │ 1528 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (wall clock) (ns) * │ 2 │ 2 │ 2 │ 2 │ 2 │ 2 │ 3 │ 1528 │
╘═══════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛with the use of unsafeBytes(..)
Reading Doubles
╒═══════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│ Metric │ p0 │ p25 │ p50 │ p75 │ p90 │ p99 │ p100 │ Samples │
╞═══════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│ Malloc (total) * │ 0 │ 0 │ 0 │ 0 │ 0 │ 0 │ 0 │ 59 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Memory (resident peak) (K) │ 8143 │ 8462 │ 8462 │ 8471 │ 8471 │ 8471 │ 8471 │ 59 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Releases * │ 3 │ 3 │ 3 │ 3 │ 3 │ 3 │ 3 │ 59 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (total CPU) (ns) * │ 50 │ 51 │ 51 │ 51 │ 51 │ 65 │ 65 │ 59 │
├───────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (wall clock) (ns) * │ 50 │ 50 │ 50 │ 51 │ 51 │ 65 │ 65 │ 59 │
╘═══════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following PR allows users to access a pointer to a vector of objects within the ByteBuffer without the need to copy or create an array until the users would want to.
UnsafeBufferPointer<T>which will always assume the type, and always provide access to a type ofT.depends on #8478