-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Further improve performance of JSON serialization on Linux #723
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
Conversation
- move expensive _SwiftValue.store case below Array, Dictionary and primitive types - add fast paths for Int and UInt types
- move NSNumber case (which uses _SwiftValue.store) below Array and Dictionary - add fast paths for Int, UInt, Float, Double
// object is a Double and is not NaN or infinity | ||
if let number = obj as? Double { | ||
let invalid = number.isInfinite || number.isNaN | ||
return !invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would return number.isFinite
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a neater way to express it, thanks. I've updated the PR,
... when checking Double and Float validity
case _ where _SwiftValue.store(obj) is NSNumber: | ||
try serializeNumber(_SwiftValue.store(obj) as! NSNumber) | ||
case let num as Int: | ||
writer(String(describing: num)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really have control over how String decides to write out this number? Wouldn't it be better to keep the exact logic inside the JSONSerialization
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation represents the value as an NSNumber
and then formats it using CFNumberFormatterCreateStringWithNumber
using a CFNumberFormatter
created by the JSONSerialization
class. It is the CF code (and format string that is passed to the formatter) determines how a number is represented.
However, this is significantly slower than using String. Do you foresee a risk that the format of an Int / UInt produced by String may change?
An alternative could be to replicate what String is doing within JSONWriter
. (is this what you had in mind?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the String(describing:)
output that I'm concerned about, yes. I would rather control the output in code more tightly coupled to the writer itself, since formatting that output exactly is basically the entire job of this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't as straightforward as I'd expected, and I need to give it some more thought. In the meantime, I'll revert the fast paths from this PR as I'd like to deliver the 20% improvement from case reordering.
...to be reintroduced later such that JSONSerialization maintains control of the formatting
@parkera I've removed the Int/UInt fastpaths for now, to be submitted in a future PR, as I'd like to deliver the 20% improvement from case reordering. Are you okay with this approach? |
@swift-ci please test |
@parkera Is there anything else needed for this to be merged? |
This should be good to go. We should also measure again once we can get #734 landed. |
@swift-ci test and merge |
Following on from #718, this change further improves performance of JSON serialization:
JSONWriter.serializeJSON
, move the expensive NSNumber case (which invokes_SwiftValue.store
) below the cases for Array, Dictionary and primitive types.isValidJSONObject
These changes were developed by @shmuelk and myself.
Additional performance can be gained by adding similar fast paths for Double and Float, however doing so fails the tests that expect a whole number to be serialized without decimal places (eg.
Double(1.0)
->1
). For that reason, I have not included a fast path for Double or Float in this PR, but I included the performance numbers below for the sake of interest.Update: I've removed the Int/UInt fastpath for now, until I can resolve the control over formatting issue raised in the review below. This PR is now just contains the 1.2x improvement (from case reordering). I'll submit the fastpaths in a future PR.
Using the benchmark I developed for #718, increased to 10,000 repetitions, I get the following results on Ubuntu 16.04:
(units are average execution time in seconds, and the code changes are cumulative).