Reducing Swift Runtime overhead when useDefaultKeys keyDecodingStrategy is used

[ChrisBenua]

Shortly, the larger app is, the more expensive protocol conformance checking is.

For detailed explanation see: https://forums.swift.org/t/improving-jsondecoder-encoder-performance-for-large-apps/81839/

I ran JSONBenchmark in Swift repo with my changes in ZippyJSONDecoder and without - there is barely any difference. At most 1% boost for some percentile and 1% degradation in other percentiles

But when I check performance using my own benchmark that is oriented on measuring Swift Runtime overhead - https://github.com/ChrisBenua/JSONDecoderEncoderBenchmarks

Here are results:

Mode: Regular CodingKeys decode_zippy
Median: 6.533840
0.25 Quantile: 6.524136
0.75 Quantile: 6.560604

Mode: Regular CodingKeys decode_zippy_new
Median: 2.767571
0.25 Quantile: 2.766739
0.75 Quantile: 2.770014

So the newer version has significantly less Swift Runtime overhead

[michaeleisel]

Thanks for the PR. I thought Apple had tried to make protocol conformance faster a couple years ago? Is it faster now, but just still a bottlebeck? I also remember it being a little finicky to measure it properly, because things like being interposing libraries (e.g. as Xcode does at launch) could cause the optimizations not to happen. @noahsmartin

In any case, this PR looks cool, and I may merge it, but I want to discuss some things. Right now, this repo is basically in maintenance mode, sort of waiting for everyone to stop using iOS 16 and below. See here: #69 . This is because I don't see further optimizations that can be made to become significantly faster than Apple's one, JSONDecoder. In fact, Apple's one is often a little faster. My recommendation, assuming that this speedup is genuine and not just for a pathological case or two (I would to need spend more time with it), is to first try to upstream it with JSONDecoder. If that becomes a big hassle, or it stalls, then I'd be more inclined to merge it, and take this repo out of maintenance mode (although Apple may just upstream it later 😓 ). But if they do merge it, then I may or may not just let this repo stay in maintenance mode and not merge the change. Are you planning to try to merge it there?

[michaeleisel]

Although, I suppose if they do make a fix, it would only apply to iOS 18+ at best? Yeah, it may be worth merging this regardless of what they do. But I want to maybe see a little more discussion on this first, and get JSONDecoder maintainers thoughts. Also, there are other optimizations you want to do, right?

[ChrisBenua]

I thought Apple had tried to make protocol conformance faster a couple years ago? Is it faster now, but just still a bottlebeck? I also remember it being a little finicky to measure it properly, because things like being interposing libraries (e.g. as Xcode does at launch) could cause the optimizations not to happen.

Yep, you're absolutely right! Apple made dyld cache for protocol conformances in iOS 16. That's why my team and I were skeptical about this optimisations at first and then we conducted AB-testing on real users of our app. And we got statistically significant results of our optimized version of JSONDecoder (literally a full copy of Foundation.JSONDecoder with similar diff as in this PR) being almost as twice as Foundation.JSONDecoder. We've collected info about 80 k launches of our app: 40 k launches with standard Foundation.JSONDecoder and 40 k launches with our optimized version. So, in my opinion, this is pretty massive sampling frame and we can assume that dyld-cache does not help to achieve great performance here.

I've already created similar PR to swift-foundation repo: https://github.com/swiftlang/swift-foundation/pull/1481/files.

It will be really great if Noah or some other folks from EmergeTools could review my small performance research!

[ChrisBenua]

Although, I suppose if they do make a fix, it would only apply to iOS 18+ at best? Yeah, it may be worth merging this regardless of what they do. But I want to maybe see a little more discussion on this first, and get JSONDecoder maintainers thoughts.

I really hope that my fix will be merged and shipped to older version of iOS 18. Surely after 18.6.2.

Also, there are other optimizations you want to do, right?

Unfortunately, It is the only optimization that I can do with ease. Removing type generic constraints here: https://github.com/michaeleisel/ZippyJSON/blob/master/Sources/ZippyJSON/ZippyJSONDecoder.swift#L834, unfortunately, won't help.

My other optimization will require breaking ABI in Foundation or introducing experimental flags to stop generating enum CodingKeys that take extra time when passed as generic parameter to KeyedDecodingContainer and waste extra ~1.8 kb of binary size. And Kevin Perry suggested that there is new approach to decoding/encoding in Swift that he is working on and it will be available in near future. So the game is not worth playing. Here is his comment: https://forums.swift.org/t/improving-jsondecoder-encoder-performance-for-large-apps/81839/6

[michaeleisel]

It looks like you've had some good suggestions on the other PR. Feel free to modify this PR as needed and let me know when you think it's ready for merge. If it passes all tests, I'll be happy to merge it and cut a release.

Also, just curious, how would it be backported? Does Apple do patch version releases (*.*.X) with upstream commits for performance issues like this one?

[ChrisBenua]

It looks like you've had some good suggestions on the other PR. Feel free to modify this PR as needed and let me know when you think it's ready for merge. If it passes all tests, I'll be happy to merge it and cut a release.

Also, just curious, how would it be backported? Does Apple do patch version releases (*.*.X) with upstream commits for performance issues like this one?

I've updated both PRs. I've extracted extension to Utilities.swift here also. I think this PR is ready to be merged if you don't mind it

Oh, that's tough question. I don't think Apple will consider this performance issue significant enough to create patch release...

[ChrisBenua]

Had to do it because now zippyJSONDecoder and regular JSONDecoder hit different code paths for dictionaries

[michaeleisel]

Sounds about right, new OS version with new error messages since the last time this was tested

[ChrisBenua]

I've fixed one test, applied new code-style. Now, for sure, PR is ready to be merged)

[michaeleisel]

Thank you! I'll make a branch with a stylistic change to this one, and merge it. A few side notes around this stuff:

• If the key decoding strategy is "custom", then ZippyJSON falls back to Apple's JSONDecoder
• There's a known bug in ZippyJSON (IIRC) where, if you create a KeyedDecodingContainer and then afterwards also decode the same thing as a dictionary, and you have conversion to .convertFromSnakeCase, then you can run into problems. This is because it does the initial conversion from snake into camel in-place, and that's what the dictionary version sees afterwards. However, no one has ever reported this as an issue, and it seems pretty obscure (I imagine someone would need to make a custom decoder to make this happen?). It's fixable, but has never seemed worth it to fix, and would have a little overhead.
• There was an experimental library for faster protocol conformance checking, but it never became production ready: https://github.com/EmergeTools/zconform

[michaeleisel]

You caught it right as I'd force-pushed to master 😅

[michaeleisel]

Merged via #72