Skip to content

Conversation

patrickfreed
Copy link
Contributor

SWIFT-1044

This PR implements a number of improvements to extended JSON generation as laid out in the design doc. Most of the code changes ended up being the original iterator improvements you wrote a while back.

benchmark results (in seconds median execution time):

flat_bson deep_bson full_bson
libbson 2.0 0.8 1.2
unoptimized 31.5 207.6 46.6
target 8.0 3.2 4.6
optimized 3.2 2.5 4.2

@patrickfreed patrickfreed marked this pull request as ready for review January 14, 2021 23:28
@patrickfreed patrickfreed requested a review from kmahar January 14, 2021 23:28
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

most of my comments here are about code I wrote myself, lol. there's kind of a broader question around what to do encountering invalid data after we've already validated, on which I lean toward defensiveness.

// We need to re-implement this using the default Sequence implementation since the default one from
// `Collection` (which `BSONDocument` also conforms to) relies on numeric indexes for iteration and is therefore
// very slow.
@inlinable
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want/need this to be inlinable? I'm actually not super well versed on that subject but from my cursory understanding (based on thread here) it only has any benefit when this method is called from other modules.
there's maybe a larger conversation for us to have and research to do around making things inlinable in our libraries, but I don't know if it's relevant to the issues at hand here since IIUC we are doing this to make code within the BSON library faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this declaration straight from Sequence.swift in the Swift standard library without really considering what it meant or did: https://github.com/apple/swift/blob/main/stdlib/public/core/Sequence.swift#L600. Having read that thread, it looks like we probably want to keep it. Note that this is the public implementation of map on BSONDocument now, so it will definitely be used across modules. Also, since users grab our source from SPM/git, I don't think we have to worry about ABI stability, since users have to recompile any of our changes anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah rereading I see that the stability concerns don't really apply to normal library authors. could you open a ticket about considering if there are any other places we should make inlinable down the road?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed SWIFT-1069

Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

lgtm!

@patrickfreed patrickfreed merged commit fdae759 into mongodb:master Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants