Skip to content

Fix perfomance issue caused by Dictionary's swift_dynamicCast#177

Merged
ikesyo merged 7 commits intoikesyo:masterfrom
yudoufu:fix-dictionary-peformance
Jun 2, 2017
Merged

Fix perfomance issue caused by Dictionary's swift_dynamicCast#177
ikesyo merged 7 commits intoikesyo:masterfrom
yudoufu:fix-dictionary-peformance

Conversation

@yudoufu
Copy link
Copy Markdown
Contributor

@yudoufu yudoufu commented May 24, 2017

When I checked Himotoki's perfomance by JSONShootout with Himotoki, the result was very slow.
It was very sad... 😢 so I inspect the issue and I found reason.

It was caused by occured swift_dynamicCast by cast to [String: Any] in valueFor.
So I fixed it to use NSDictionary for this cast, and the benchmark result changed very fast. 😄

I want to review and merge this fix.

2017-05-24 23 09 31

return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

#endif

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

#endif

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)


#if os(macOS) || os(iOS) || os(tvOS) || os(watchOS)
import class Foundation.NSDictionary
private typealias _Dictionary = NSDictionary
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Type Name Violation: Type name should only contain alphanumeric characters: '_Dictionary' (type_name)

@ikesyo ikesyo force-pushed the fix-dictionary-peformance branch from ce40ba6 to 0c7ffc8 Compare June 2, 2017 05:03
@ikesyo
Copy link
Copy Markdown
Owner

ikesyo commented Jun 2, 2017

I'm sorry for the late response and really appreciate your contribution @yudoufu! ✨

I've slightly modified the pull request to cleanly introduce your proposed changes. Please let me know if you have further opinion against this. If you are okay I'll merge this and cut a new release.

@yudoufu
Copy link
Copy Markdown
Contributor Author

yudoufu commented Jun 2, 2017

@ikesyo Thank you for review and fix to my opinion.
I'm agree for your fix. It's make the most sense. 👍

@ikesyo
Copy link
Copy Markdown
Owner

ikesyo commented Jun 2, 2017

:shipit:

@ikesyo ikesyo merged commit c8b0306 into ikesyo:master Jun 2, 2017
@dcaunt
Copy link
Copy Markdown

dcaunt commented Jun 2, 2017

Awesome!

@yudoufu do you intend to submit a PR to JSONShootout?

@yudoufu
Copy link
Copy Markdown
Contributor Author

yudoufu commented Jun 2, 2017

Yes, I will do it!
Thank you for comment @dcaunt :)

@ikesyo ikesyo mentioned this pull request Jun 6, 2017
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.

4 participants