Added fast path for Int and UInt #923
Conversation
Foundation/NSJSONSerialization.swift
Outdated
| var array: [UInt] = [] | ||
| var stringResult = "" | ||
| //Maximum length of an UInt | ||
| array.reserveCapacity(20) |
There was a problem hiding this comment.
This value of 20 seems magical. Can we formalize it somehow?
There was a problem hiding this comment.
I think we could make these constants like this:
let maxUIntLength = String(describing: UInt.max).characters.count
let maxIntLength = String(describing: Int.max).characters.countWe'd need to calculate these once (not every time we serialize), so make them private properties of NSJSONSerialization.
There was a problem hiding this comment.
I've also made these changes, you can find them in the new commit.
Is this an acceptable way of formalizing it @parkera ?
Foundation/NSJSONSerialization.swift
Outdated
| var array: [Int] = [] | ||
| var stringResult = "" | ||
| //Maximum length of an Int | ||
| array.reserveCapacity(19) |
| @@ -1054,6 +1058,26 @@ extension TestNSJSONSerialization { | |||
| XCTAssertEqual(try trySerialize(json), "[false,true]") | |||
| } | |||
|
|
|||
| func test_serialize_IntMax() { | |||
| let json: [Any] = [Int.max] | |||
There was a problem hiding this comment.
Is Int.max architecture specific?
There was a problem hiding this comment.
Yes. Documentation:
On 32-bit platforms, Int is the same size as Int32, and on 64-bit platforms, Int is the same size as Int64.
So I think this will break on 32-bit platforms. We should construct the string we're asserting against using the same constant, I think. The same applies for the tests which use Int.min, UInt.max and UInt.min.
There was a problem hiding this comment.
Thanks for finding that, I've updated the tests to compare against a string representation of the Int.max value so this should also work on 32-bit platforms now.
| case 9, -9: stringResult.append("9") | ||
| default: fatalError() | ||
| } | ||
| } |
There was a problem hiding this comment.
Though the loops where we walk backwards are not exactly identical in the two functions, I am wondering if we can still refactor them into a single function.
There was a problem hiding this comment.
Thanks for the feedback, it is possible to refactor them into a single function however both Ian and myself felt the code looked more complicated and required a little more work in terms of casting and such, so I've omitted it for now.
|
@parkera Following your comments I've made some changes, are these changes acceptable? Thanks. |
|
Thanks! |
|
@swift-ci test and merge |
As a follow on from #723 I've created fast path options for Int and UInt to further improve the performance of JSON Serialization.
Below are some results I obtained by using a slightly modified version of the benchmark @djones6 developed, increased to 1000 repetitions and varying the payload size. I've included times for
String(describing:)as that was the original proposed solution in #723 and felt it would offer a good comparative view.master)@djones6 and I also ran some tests against Kitura, these tests showed we are slightly better than
String(describing:)for shorter numbers and on par withString(describing:)for longer numbers.I've also added a few extra tests for serialization of the min/max values for Int and UInt as I felt these offered good values for testing the correct result is produced, also the current master implementation fails on max UInt.
I haven't started work on Double/Floats yet but may do so if this fix is acceptable.