search

swift-server / swift-aws-lambda-runtime

Swift implementation of AWS Lambda Runtime
Apache License 2.0
1.13k stars 102 forks source link

Consider replacing Foundation JSON encoder #153

Closed pokryfka closed 3 years ago

pokryfka commented 4 years ago

Given that:

Consider:

Performance comparison:

From PureJSONSwift

Encoding

macOS Swift 5.1 macOS Swift 5.2 Linux Swift 5.1 Linux Swift 5.2
Foundation 2.61s 2.62s 13.03s 12.52s
PureSwiftJSON 1.23s 1.25s 1.13s 1.05s
Speedup ~2x ~2x ~10x ~10x

Decoding

macOS Swift 5.1 macOS Swift 5.2 Linux Swift 5.1 Linux Swift 5.2
Foundation 2.72s 3.04s 10.27s 10.65s
PureSwiftJSON 1.70s 1.72s 1.39s 1.16s
Speedup ~1.5x ~1.5x ~7x ~8x

From aws-xray-sdk-swift

(Average) results in seconds of EncodingTests from run 191224345

Test test-macos (5.2.4) test-linux (swift:5.2) test-linux (swiftlang/swift:nightly-5.3-bionic)
testEncodingUsingFoundationJSON 0.884 2.258 2.293
testEncodingUsingIkigaJSON 0.518 0.262 0.248
testEncodingUsingPureSwiftJSON 0.497 0.347 0.328
pokryfka commented 4 years ago

@kneekey23 let's rerun the tests ;-)

tomerd commented 4 years ago

another thing that can help performance are static linking and/or making sure the zipfile only contains that absolutely necessary dylibs

tomerd commented 3 years ago

note: foundation JSON has been getting some performance boosts from @fabianfett and @spevans

https://github.com/apple/swift-corelibs-foundation/pull/2985 https://github.com/apple/swift-corelibs-foundation/pull/2987 https://github.com/apple/swift-corelibs-foundation/pull/2988 https://github.com/apple/swift-corelibs-foundation/pull/2989

fabianfett commented 3 years ago

@pokryfka given that the json performance has gotten better on Linux (if you use the main nightly build 😉), Foundation is a sensible preset, and there is an API to use your own Encoder/Decoder, do you think it is okay to close this issue?

pokryfka commented 3 years ago

@fabianfett thank you for the update and great to hear about the improvements!

Did you happen to make any performance tests before and after the changes? for example using https://github.com/swift-extras/swift-extras-json/tree/main/PerfTests

I take your word that it improved :-), I think it is okay to close it.

On side note: It's really great that Foundation JSON encoder/decoder performance improved and all swift-server should benefit. In case of lambdas ... probably it will always be desirable to avoid Foundation to improve cold start (as much as possible, AHC and Soto use Foundation primarily for its URL and Date types). (I think there's been some improvements with static linking on Linux side which might have helped a bit).

spevans commented 3 years ago

@pokryfka

In case of lambdas ... probably it will always be desirable to avoid Foundation to improve cold start

As you note static linking is definitely a help and obviously avoiding FoundationNetworking is a big win to avoid all of the libcurl dynamic libraries, but if you have any specific examples of it being slow it could possibly be investigated to see if there are any possible fixes - A new issue might be more appropriate though.

pokryfka commented 3 years ago

@pokryfka

In case of lambdas ... probably it will always be desirable to avoid Foundation to improve cold start

As you note static linking is definitely a help and obviously avoiding FoundationNetworking is a big win to avoid all of the libcurl dynamic libraries, but if you have any specific examples of it being slow it could possibly be investigated to see if there are any possible fixes - A new issue might be more appropriate though.

Agreed its not related to JSON performance as such.

From my limited testing some time ago dynamic loading had its penalty (I managed to statically link lambda function but not when using Foundation, I think there has been some updates in this area recently, planning to retest it).

The other issue was big size of the package, affecting bootstrapping by AWS during cold start; the biggest contributor here, bigger thanFoundation, is ICU as mentioned in https://github.com/swift-server/swift-aws-lambda-runtime#performance

spevans commented 3 years ago

If a binary is compiled using -static-executable then it should just be demand paged when used and should have quite a small cold start time.

ICU is mostly CLDR data used for uncicode/localisation/date time etc and is also used by the stdlib so will always be present. Its hard to shrink the data down but https://bugs.swift.org/browse/SR-9432 is probably one to watch as that may help in that direction.

Lastly, if libxml2 is used in anyway that uses the system version and at least on Ubuntu that is linked to the system ICU so actually two versions of ICU are then loaded into memory. Im not sure how its built on Amazon Linux (ldd would tell you) but vending a libxml2 as part of the swift toolchain linked to the vended ICU library would also help.

spevans commented 3 years ago

I did some benchmarks testing the new JSONDecoder v older toolchains and added in IkigaJSON for a general test as well.

Note the improvements from 5.3.3 to 5.4-DEVELOPMENT are purely due to compiler/runtime/stdlib improvements, there are no specific JSONDecoder changes in 5.4. So it looks like the overall decrease in time by 26% is due to ARC and other improvements.

There is still some work ongoing to fix number parsing which does have some issues, but overall the numbers look more encouraging for the new decoder.

pokryfka commented 3 years ago

I did some benchmarks testing the new JSONDecoder v older toolchains and added in IkigaJSON for a general test as well.

Note the improvements from 5.3.3 to 5.4-DEVELOPMENT are purely due to compiler/runtime/stdlib improvements, there are no specific JSONDecoder changes in 5.4. So it looks like the overall decrease in time by 26% is due to ARC and other improvements.

There is still some work ongoing to fix number parsing which does have some issues, but overall the numbers look more encouraging for the new decoder.

Thank you for sharing, this looks very good!