Skip to content

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

Merged
merged 4 commits into from
Dec 12, 2016
Merged

Further improve performance of JSON serialization on Linux #723

merged 4 commits into from
Dec 12, 2016

Conversation

djones6
Copy link
Contributor

@djones6 djones6 commented Nov 23, 2016

Following on from #718, this change further improves performance of JSON serialization:

  • In JSONWriter.serializeJSON, move the expensive NSNumber case (which invokes _SwiftValue.store) below the cases for Array, Dictionary and primitive types.
  • add fast paths for Int and UInt types, which can be converted directly to a String rather than going via NSNumber.
  • Equivalent changes for 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:

Payload type:       Int   (speedup)        Double   (speedup)

Baseline:           8.72                   8.78
With #718:          4.18  (2.1x)           3.97   (2.2x)
Reorder case:       3.43  (1.2x)           3.29   (1.2x)
Int/UInt fastpath:  2.33  (1.5x)           ----

(Double fastpath):  ----                   2.42   (1.35x)

(units are average execution time in seconds, and the code changes are cumulative).

- 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
Copy link
Contributor

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?

Copy link
Contributor Author

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))
Copy link
Contributor

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?

Copy link
Contributor Author

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?)

Copy link
Contributor

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.

Copy link
Contributor Author

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
@djones6
Copy link
Contributor Author

djones6 commented Dec 2, 2016

@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?

@parkera
Copy link
Contributor

parkera commented Dec 2, 2016

@swift-ci please test

@djones6
Copy link
Contributor Author

djones6 commented Dec 12, 2016

@parkera Is there anything else needed for this to be merged?

@parkera
Copy link
Contributor

parkera commented Dec 12, 2016

This should be good to go. We should also measure again once we can get #734 landed.

@parkera
Copy link
Contributor

parkera commented Dec 12, 2016

@swift-ci test and merge

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