Skip to content

Add a fast-path case for enumeration of keys and objects for bridged Dictionaries #6421

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.

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 1 commit into from
Mar 28, 2017

Conversation

phausler
Copy link
Contributor

This adds a fast-path accessor for enumeration of the bridged representation of Dictionaries such that Foundation can take advantage of that particular fast-path when bridging property list types

Custom benchmark of profiling 10000 property lists from /Applications shows that the average time per plist to convert back to data before this change is approximately 0.019s on the machine I tested with. After the change (with the corresponding Foundation change) it is approximately 0.0094s (mileage and exact numbers subject to change per hardware). This will not be in effect until Foundation fully adopts the proposed changes required for taking advantage of this as a fast-path (so no current benchmarks should show any markable difference)

@phausler
Copy link
Contributor Author

@swift-ci please test


@objc(enumerateKeysAndObjectsWithOptions:usingBlock:)
internal func enumerateKeysAndObjects(options: Int,
using block: @convention(block) (Unmanaged<AnyObject>, Unmanaged<AnyObject>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can leave out the Unmanaged here; it's understood that blocks take arguments at +0.

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 will take a look at that tomorrow to verify the behavior is correct per Foundation changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping! I was just looking at this again over the weekend, and this would be a huge win

@phausler
Copy link
Contributor Author

I thought I had merged this already, the required changes for Foundation have already landed so this will now be an applicable improvement going forward.

@phausler phausler merged commit bb674fe into swiftlang:master Mar 28, 2017
@jrose-apple
Copy link
Contributor

Ah, in the future can you retest when merging something so old? I can't see how this particular patch would cause any harm but just in case.

@phausler
Copy link
Contributor Author

Ok will do.
Figured it was so isolated that it wouldn't cause any issue (if it did I would have claimed any failure to be a regression...)

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.

3 participants