Skip to content

SWIFT-411 Make pointer access to bson_ts more explicit #264

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 21, 2019
Merged

SWIFT-411 Make pointer access to bson_ts more explicit #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
877d5db
use withMutableBSONPointer helper everywhere, rename internal properties
kmahar May 16, 2019
68d3fbd
no need to copy error
kmahar May 17, 2019
5b10567
comment/cleanup
kmahar May 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions Sources/MongoSwift/BSON/BSONValue.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ extension Array: BSONValue {
try arr.setValue(for: String(i), to: val)
}

guard bson_append_array(storage.pointer, key, Int32(key.utf8.count), arr.data) else {
Copy link
Contributor Author

@kmahar kmahar May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it occurred to me as I made these changes that we are kind of doing the same bad thing here where we are mutating this bson_t. but maybe it's overkill to do some kind of withPointer for the storage, too.
similar to DocumentIterator, DocumentStorage could probably use some rethinking...

Copy link
Contributor

@patrickfreed patrickfreed May 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It's making me think we should just make DocumentStorage a private type (not even internal) and update all the public or non-Document stuff that uses it just take a Document instead. It seems too dangerous to circumvent the api you're adding to Document in this PR to warrant exposing the storage, even at the internal level.

Copy link
Contributor Author

@kmahar kmahar May 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that.... it seems like it might confuse users that there would be this public method on BSONValue:
encode(to: Document, forKey key: String)
but that we don't actually intend for them to set values on documents that way....

then again, this existence of that method and DocumentStorage in the public API are already confusing as it is.

I wonder if DocumentStorage should get a bunch of methods on it: appendInt32, appendString, and so on... and then we type switch within Document (or in some generic appendBSONValue method on DocumentStorage) to decide which method to call?

that brings us back to same problem where BSONValue is a protocol but people can't really actually make arbitrary types conform to it since we wouldn't be switching on them there...

Copy link
Contributor

@patrickfreed patrickfreed May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we call copyStorageIfRequired where necessary in setValue and we've updated the count property, the bad things we're doing here aren't resulting in any buggy behavior (I think). It would be preferable to guarantee that at the mutating call's site, but without big changes that'll be hard to do for the reasons you mention.

As far as big changes go though, I have a fun idea...

We would update DocumentStorage to be a wrapper around an internal Data buffer that we manage ourselves instead of a bson_t. We'd then update the BSONValue protocol to require a bson: Data property (or something like that) that emits a buffer containing the raw BSON bytes (and remove encode(to:forKey). Then, where we normally call encode, we instead get this buffer from the BSONValue and append it to our internal buffer (with the necessary bookkeeping for keys, types, etc). When we do need a bson_t, we can use bson_init_static to create a lightweight, read-only, and stack allocated bson_t from the internal buffer.

This gets us like 99% of the way to pure Swift BSON, avoids a lot of memory issues, and makes the BSONValue protocol more sensical. This is obviously out of scope for this ticket, but I think it could be a good approach for solving issues we have now as well as moving towards a pure Swift driver.

Copy link
Contributor Author

@kmahar kmahar May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, when I started thinking about the pure Swift BSON library design I came up with something very similar where we would have a bytes property on BSONValue 🙂

Copy link
Contributor

@patrickfreed patrickfreed May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me realize that pure swift BSON library may actually be a breaking change, so we might have to do it pre-GA 🤔

Copy link
Member

@mbroadst mbroadst May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we discussed this some time ago. This is what the C++ driver does, rather than keeping instances of bson_t everywhere. bson_t initialization isn't zero overhead, but it might not be that bad. I'd say go ahead and make a ticket for the feature

Copy link
Contributor

@patrickfreed patrickfreed May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed SWIFT-497

guard bson_append_array(storage._bson, key, Int32(key.utf8.count), arr._bson) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -255,7 +255,7 @@ public struct BSONNull: BSONValue, Codable, Equatable {
}

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_null(storage.pointer, key, Int32(key.utf8.count)) else {
guard bson_append_null(storage._bson, key, Int32(key.utf8.count)) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -356,7 +356,7 @@ public struct Binary: BSONValue, Equatable, Codable {
let subtype = bson_subtype_t(UInt32(self.subtype))
let length = self.data.count
let byteArray = [UInt8](self.data)
guard bson_append_binary(storage.pointer, key, Int32(key.utf8.count), subtype, byteArray, UInt32(length)) else {
guard bson_append_binary(storage._bson, key, Int32(key.utf8.count), subtype, byteArray, UInt32(length)) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -392,7 +392,7 @@ extension Bool: BSONValue {
public var bsonType: BSONType { return .boolean }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_bool(storage.pointer, key, Int32(key.utf8.count), self) else {
guard bson_append_bool(storage._bson, key, Int32(key.utf8.count), self) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -422,7 +422,7 @@ extension Date: BSONValue {
public var msSinceEpoch: Int64 { return Int64((self.timeIntervalSince1970 * 1000.0).rounded()) }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_date_time(storage.pointer, key, Int32(key.utf8.count), self.msSinceEpoch) else {
guard bson_append_date_time(storage._bson, key, Int32(key.utf8.count), self.msSinceEpoch) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -464,7 +464,7 @@ public struct DBPointer: BSONValue, Codable, Equatable {

public func encode(to storage: DocumentStorage, forKey key: String) throws {
try withUnsafePointer(to: id.oid) { oidPtr in
guard bson_append_dbpointer(storage.pointer, key, Int32(key.utf8.count), self.ref, oidPtr) else {
guard bson_append_dbpointer(storage._bson, key, Int32(key.utf8.count), self.ref, oidPtr) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -538,7 +538,7 @@ public struct Decimal128: BSONNumber, Equatable, Codable, CustomStringConvertibl

public func encode(to storage: DocumentStorage, forKey key: String) throws {
try withUnsafePointer(to: self.decimal128) { ptr in
guard bson_append_decimal128(storage.pointer, key, Int32(key.utf8.count), ptr) else {
guard bson_append_decimal128(storage._bson, key, Int32(key.utf8.count), ptr) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -601,7 +601,7 @@ extension Double: BSONNumber {
public var bsonType: BSONType { return .double }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_double(storage.pointer, key, Int32(key.utf8.count), self) else {
guard bson_append_double(storage._bson, key, Int32(key.utf8.count), self) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -681,7 +681,7 @@ extension Int32: BSONNumber {
public var bsonType: BSONType { return .int32 }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_int32(storage.pointer, key, Int32(key.utf8.count), self) else {
guard bson_append_int32(storage._bson, key, Int32(key.utf8.count), self) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -711,7 +711,7 @@ extension Int64: BSONNumber {
public var bsonType: BSONType { return .int64 }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_int64(storage.pointer, key, Int32(key.utf8.count), self) else {
guard bson_append_int64(storage._bson, key, Int32(key.utf8.count), self) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -764,11 +764,11 @@ public struct CodeWithScope: BSONValue, Equatable, Codable {

public func encode(to storage: DocumentStorage, forKey key: String) throws {
if let s = self.scope {
guard bson_append_code_with_scope(storage.pointer, key, Int32(key.utf8.count), self.code, s.data) else {
guard bson_append_code_with_scope(storage._bson, key, Int32(key.utf8.count), self.code, s._bson) else {
throw bsonTooLargeError(value: self, forKey: key)
}
} else {
guard bson_append_code(storage.pointer, key, Int32(key.utf8.count), self.code) else {
guard bson_append_code(storage._bson, key, Int32(key.utf8.count), self.code) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -811,7 +811,7 @@ public struct MaxKey: BSONValue, Equatable, Codable {
public var bsonType: BSONType { return .maxKey }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_maxkey(storage.pointer, key, Int32(key.utf8.count)) else {
guard bson_append_maxkey(storage._bson, key, Int32(key.utf8.count)) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -842,7 +842,7 @@ public struct MinKey: BSONValue, Equatable, Codable {
public var bsonType: BSONType { return .minKey }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_minkey(storage.pointer, key, Int32(key.utf8.count)) else {
guard bson_append_minkey(storage._bson, key, Int32(key.utf8.count)) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -925,7 +925,7 @@ public struct ObjectId: BSONValue, Equatable, CustomStringConvertible, Codable {
public func encode(to storage: DocumentStorage, forKey key: String) throws {
// encode the bson_oid_t to the bson_t
try withUnsafePointer(to: self.oid) { oidPtr in
guard bson_append_oid(storage.pointer, key, Int32(key.utf8.count), oidPtr) else {
guard bson_append_oid(storage._bson, key, Int32(key.utf8.count), oidPtr) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -1044,7 +1044,7 @@ public struct RegularExpression: BSONValue, Equatable, Codable {
}

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_regex(storage.pointer, key, Int32(key.utf8.count), self.pattern, self.options) else {
guard bson_append_regex(storage._bson, key, Int32(key.utf8.count), self.pattern, self.options) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -1077,7 +1077,7 @@ extension String: BSONValue {
public var bsonType: BSONType { return .string }

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_utf8(storage.pointer, key, Int32(key.utf8.count), self, Int32(self.utf8.count)) else {
guard bson_append_utf8(storage._bson, key, Int32(key.utf8.count), self, Int32(self.utf8.count)) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -1135,7 +1135,7 @@ public struct Symbol: BSONValue, CustomStringConvertible, Codable, Equatable {

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_symbol(
storage.pointer,
storage._bson,
key,
Int32(key.utf8.count),
self.stringValue,
Expand Down Expand Up @@ -1191,7 +1191,7 @@ public struct Timestamp: BSONValue, Equatable, Codable {
}

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_timestamp(storage.pointer, key, Int32(key.utf8.count), self.timestamp, self.increment) else {
guard bson_append_timestamp(storage._bson, key, Int32(key.utf8.count), self.timestamp, self.increment) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down Expand Up @@ -1227,7 +1227,7 @@ public struct BSONUndefined: BSONValue, Equatable, Codable {
}

public func encode(to storage: DocumentStorage, forKey key: String) throws {
guard bson_append_undefined(storage.pointer, key, Int32(key.utf8.count)) else {
guard bson_append_undefined(storage._bson, key, Int32(key.utf8.count)) else {
throw bsonTooLargeError(value: self, forKey: key)
}
}
Expand Down
Loading