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

Improve JSON serialization performance on Linux #718

Merged
merged 3 commits into from Nov 24, 2016

Conversation

Projects
None yet
3 participants
@djones6
Contributor

djones6 commented Nov 17, 2016

This change improves the performance of serializing objects to a Data, by using String concatenation to build the result, and then convert to a Data once. This change was developed by @shmuelk and myself.

We may want to revisit this in the future as the performance of appending to Data improves.

Note, we initially used jsonStr.cString(using: .utf8) to create the Data, but in testing this seemed to leak memory (perhaps this is a bug): IBM-Swift/SwiftyJSON#22

I've provided a microbenchmark to demonstrate the effect of these changes:

Before:

TestFoundation/TestNSJSONSerialization.swift:760: Test Case 'TestNSJSONSerialization.test_serialization_perf' measured [Time, seconds] average: 0.463, relative standard deviation: 3.143%, values: [0.485122, 0.469292, 0.439064, 0.447310, 0.455177, 0.482358, 0.455306, 0.468886, 0.468181, 0.461310], performanceMetricID:org.swift.XCTPerformanceMetric_WallClockTime, maxPercentRelativeStandardDeviation: 10.000%, maxStandardDeviation: 0.100
Test Case 'TestNSJSONSerialization.test_serialization_perf' passed (4.632 seconds).

After:

TestFoundation/TestNSJSONSerialization.swift:760: Test Case 'TestNSJSONSerialization.test_serialization_perf' measured [Time, seconds] average: 0.238, relative standard deviation: 8.555%, values: [0.235258, 0.234057, 0.224006, 0.257510, 0.248202, 0.241663, 0.209058, 0.259959, 0.204592, 0.261825], performanceMetricID:org.swift.XCTPerformanceMetric_WallClockTime, maxPercentRelativeStandardDeviation: 10.000%, maxStandardDeviation: 0.100
Test Case 'TestNSJSONSerialization.test_serialization_perf' passed (2.376 seconds).

The above results were collected on Ubuntu 16.04. I obtained similar results on OS X.

Show outdated Hide outdated TestFoundation/TestNSJSONSerialization.swift
_ = group.wait(timeout: .distantFuture) // forever
}
}

This comment has been minimized.

@parkera

parkera Nov 17, 2016

Member

Thanks for including a performance test.

I'd like to split it out into some other place. The unit test is to verify correctness in automation. As this lacks an assert, it wouldn't even really catch any regressions in reality.

@parkera

parkera Nov 17, 2016

Member

Thanks for including a performance test.

I'd like to split it out into some other place. The unit test is to verify correctness in automation. As this lacks an assert, it wouldn't even really catch any regressions in reality.

This comment has been minimized.

@djones6

djones6 Nov 18, 2016

Contributor

I wasn't sure where Foundation performance tests belong. Would the Swift Benchmark Suite be a better place? I'm happy to remove it from this PR and submit a separate one.

@djones6

djones6 Nov 18, 2016

Contributor

I wasn't sure where Foundation performance tests belong. Would the Swift Benchmark Suite be a better place? I'm happy to remove it from this PR and submit a separate one.

This comment has been minimized.

@parkera

parkera Nov 18, 2016

Member

We don't really have the infrastructure yet, but we should set something up.

I think we should start a discussion on swift-dev mailing list and see if the benchmark suite is the right place or not. If not, we can create our own as part of the project.

@parkera

parkera Nov 18, 2016

Member

We don't really have the infrastructure yet, but we should set something up.

I think we should start a discussion on swift-dev mailing list and see if the benchmark suite is the right place or not. If not, we can create our own as part of the project.

let bufferLength = count+1 // Allow space for null terminator
var utf8: [CChar] = Array<CChar>(repeating: 0, count: bufferLength)
if !jsonStr.getCString(&utf8, maxLength: bufferLength, encoding: .utf8) {
fatalError("Failed to generate a CString from a String")

This comment has been minimized.

@parkera

parkera Nov 17, 2016

Member

I realize we had a force-unwrap here in the previous code, but why would getCString fail here? If it did, is there a way to recover instead of asserting?

@parkera

parkera Nov 17, 2016

Member

I realize we had a force-unwrap here in the previous code, but why would getCString fail here? If it did, is there a way to recover instead of asserting?

This comment has been minimized.

@djones6

djones6 Nov 18, 2016

Contributor

Good question. The API documentation for Swift String.getCString does not specify what the return value means, but I gather from the implementation that it calls through to NSString.getCString, the documentation for which says that false would be returned if the buffer is too small, or an encoding error occurs.
In this case, we sized the buffer appropriately, and because UTF8 can represent all Unicode code points, we shouldn't encounter an encoding error. Perhaps it would be safe to ignore the return value in this case?

@djones6

djones6 Nov 18, 2016

Contributor

Good question. The API documentation for Swift String.getCString does not specify what the return value means, but I gather from the implementation that it calls through to NSString.getCString, the documentation for which says that false would be returned if the buffer is too small, or an encoding error occurs.
In this case, we sized the buffer appropriately, and because UTF8 can represent all Unicode code points, we shouldn't encounter an encoding error. Perhaps it would be safe to ignore the return value in this case?

This comment has been minimized.

@parkera

parkera Nov 18, 2016

Member

If we're sure it should not happen, then asserting on that is the right answer instead of ignoring.

@parkera

parkera Nov 18, 2016

Member

If we're sure it should not happen, then asserting on that is the right answer instead of ignoring.

@parkera

I think that using Data will be faster than this pretty soon, but we can put a workaround like this in place for the short term.

@djones6

This comment has been minimized.

Show comment
Hide comment
@djones6

djones6 Nov 21, 2016

Contributor

@parkera I've removed the performance test from this PR, and I posted to the swift-corelibs-dev mailing list re. where it belongs.

Contributor

djones6 commented Nov 21, 2016

@parkera I've removed the performance test from this PR, and I posted to the swift-corelibs-dev mailing list re. where it belongs.

@djones6

This comment has been minimized.

Show comment
Hide comment
@djones6

djones6 Nov 23, 2016

Contributor

@parkera Assuming you're happy with the changes, is there still time to get this (and #723) considered for inclusion in the 3.0.2 release?

Contributor

djones6 commented Nov 23, 2016

@parkera Assuming you're happy with the changes, is there still time to get this (and #723) considered for inclusion in the 3.0.2 release?

@parkera

This comment has been minimized.

Show comment
Hide comment
@parkera

parkera Nov 24, 2016

Member

@swift-ci please test and merge

Member

parkera commented Nov 24, 2016

@swift-ci please test and merge

@parkera

This comment has been minimized.

Show comment
Hide comment
@parkera

parkera Nov 24, 2016

Member

Let's get this into master and then I'll check on the Swift 3.0.2 question.

Member

parkera commented Nov 24, 2016

Let's get this into master and then I'll check on the Swift 3.0.2 question.

@swift-ci swift-ci merged commit 108a5b0 into apple:master Nov 24, 2016

1 check was pending

Test and Merge Build started.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment