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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix perfomance issue caused by Dictionary's swift_dynamicCast #177

Merged
merged 7 commits into from Jun 2, 2017

Conversation

Projects
None yet
4 participants
@yudoufu
Contributor

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

Show outdated Hide outdated Sources/Extractor.swift
if result is NSNull {
return nil
}

This comment has been minimized.

@houndci-bot

houndci-bot May 24, 2017

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

@houndci-bot

houndci-bot May 24, 2017

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

Show outdated Hide outdated Sources/Extractor.swift
}

This comment has been minimized.

@houndci-bot

houndci-bot May 24, 2017

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

@houndci-bot

houndci-bot May 24, 2017

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

yudoufu added some commits May 24, 2017

Show outdated Hide outdated Sources/Extractor.swift
}

This comment has been minimized.

@houndci-bot

houndci-bot May 24, 2017

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

@houndci-bot

houndci-bot May 24, 2017

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

Show outdated Hide outdated Sources/Extractor.swift
@@ -8,13 +8,20 @@
import class Foundation.NSNull
#if os(macOS) || os(iOS) || os(tvOS) || os(watchOS)
import class Foundation.NSDictionary
private typealias _Dictionary = NSDictionary

This comment has been minimized.

@houndci-bot

houndci-bot Jun 2, 2017

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

@houndci-bot

houndci-bot Jun 2, 2017

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

@ikesyo

This comment has been minimized.

Show comment
Hide comment
@ikesyo

ikesyo Jun 2, 2017

Owner

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.

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

This comment has been minimized.

Show comment
Hide comment
@yudoufu

yudoufu Jun 2, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
Owner

ikesyo commented Jun 2, 2017

:shipit:

@ikesyo ikesyo merged commit c8b0306 into ikesyo:master Jun 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@dcaunt

This comment has been minimized.

Show comment
Hide comment
@dcaunt

dcaunt Jun 2, 2017

Awesome!

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

dcaunt commented Jun 2, 2017

Awesome!

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

@yudoufu

This comment has been minimized.

Show comment
Hide comment
@yudoufu

yudoufu Jun 2, 2017

Contributor

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

Contributor

yudoufu commented Jun 2, 2017

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

@ikesyo ikesyo referenced this pull request Jun 6, 2017

Merged

Release 3.1.0 #184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment