-
Notifications
You must be signed in to change notification settings - Fork 558
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
Reduce how much code is generated #745
base: master
Are you sure you want to change the base?
Conversation
@dtolnay Rebased and re-ran my criterion hack for "json-benchmark", there is some variance from run to run but it seems like this may actually improve performance (at least improvements seem more common and larger than any regressions in the variance). Any chance this can get merged?
|
@dtolnay are you able to take a look at this? |
👍 this would be great to have, especially in crates that depend on crates with with lots and lots of |
Should hopefully reduce the amount of code being generated
Avoids generating duplicates of the map_err per key/value type
@dtolnay Are you able to take a look at this? Is it something that might be merged at some point? |
This reduces the overall compile time of the gltf crate by about ~50-55% in my testing. (and the compilation of the extremely heavy gltf-json part where all the serde stuff lives by ~2/3) |
I did 5 runs of json benchmark for both the current master branch and this PR on my home Linux server with everything I have running on it disabled (0.00 average load, 3950x CPU, stayed sub 60C the entire time so no thermal throttling, rust 1.63, sccache disabled, For this PR, canada.json was ~8% faster, citm_catalog.json was ~6% slower, and twitter.json was basically the same. Build times were slightly faster as well, though it's dominated by the jemalloc-sys build script. The final json-benchmark bin compile time was reduced by about 1s (from ~8s to ~7s). Averages Current 44d9c53
This PR 60e4ac2
Differences (PR / Current)
|
Kindest little bump - what is the status of this PR? |
The PR has some conflicts now, but nothing that would be difficult to fix. This is still something I'd like to see merged but in the end it is up to the time and interest of the owner(s). |
An attempt to replicate the wins in #687 without using unsafe or losing any performance. To achieve this, all commonly duplicated methods have been extracted to less generic methods (only generic on
R
) which parse up to the visit method and returns anenum
to indicate how to proceed. Specifically these enums hold theError
themselves instead of being wrapped in a `Result´ since that helps codegen slightly.I unfortunately only have access to a laptop prone to throttling during benchmarking atm, so I don't have reliable measurements but this does seem to give no difference in performance or a couple of percent slowdown so I'd appreciate if someone could attempt to run this independently. (To get some more precise measurements in
json-benchmark
I hacked in criterion which can run with this commandcargo run --release --features lib-serde,all-files,parse-struct,parse -dom,serde_json,criterion --no-default-features -- --bench
)Before
After