Some performance improvements

[weiznich]

• Replace the IndexMap in the serialized object with a plain
  Vec<(String, Value)> because linear search is faster for few
  elements

• Some general tweaks to skip some allocations

Addresses #199

[LegNeato]

@theduke

[weiznich]

The ci fails because something on the master branch is brocken for the juniper_iron test case.

[LegNeato]

Hmmm, looks like this uses impl trait, which didn't stabilize until 1.26. Supporting impl trait would mean we would have to bump our min rust version.

[theduke]

Regarding versions, I'd like to stick to the stable + last 2 versions rule we have right now, so impl trait would have to wait for 1.28.

Also, we'd really need some solid benchmarks comparing the two implementations.

This might be a good oppurtunity to set up a nice criterion based benchmark suite.
It also might be an opportunity to contribute to/ improve the indexmap crate.

[theduke]

Either way, as a first step, it might be a cool idea to export a wrapper type from a submodule so we can easily change the implementation without a lot of hassle.

[weiznich]

(I'm back from vacation, so here are some answers.)

Regarding versions, I'd like to stick to the stable + last 2 versions rule we have right now, so impl trait would have to wait for 1.28.

It should be possible to do this without impl trait. I will fix the code in the next few days.

Also, we'd really need some solid benchmarks comparing the two implementations.

I've developed this change as part of optimizing wundergraph. For this I used the following benchmark. See the wundergraph repo (wundergraph-bench) for commits without and with this change. The relevant graphql query is this one:

query tracks_media_all {
  Tracks {
    id
    name
    media_type_id {
      name
    }
  }
}

Benchmark results without this change:

Benchmark results with this change:

This might be a good oppurtunity to set up a nice criterion based benchmark suite.

My benchmark is unfortunately something that involves a quite complex interplay of several components so it seems not to be appropriated to be used as benchmark for juniper alone.

It also might be an opportunity to contribute to/ improve the indexmap crate.

This is not something that could be improved in the indexmap crate.
Basically this PR changes 3 things:

1. It introduces a wrapper type for the object enum kind. This is a purely cosmetic change to give us room to change the internal presentation later.
2. It forces every construction of that wrapper type to preallocate a fixed number of elements. This eliminates the need of continuously reallocating space for more elements as soon as we start to push more field value pairs into the object. (We know how many fields we need in every place so we could just allocate the right number of slots). This contributes ~50% of the performance improvements.
3. It replaces the use of IndexMap<String, Value> with a plain Vec<(String, Value)>. On the first look this change is making things worse because now we need to to a linear search instead of a hash lookup if we want to know if some field exists. But in my observation for almost all cases that I've seen there are only a few fields (< ~50) so a linear search is faster than doing the hash lookup. This contributes the other ~50%.

[codecov-io]

Codecov Report

Merging #202 into master will increase coverage by 0.17%.
The diff coverage is 96.57%.

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   89.76%   89.93%   +0.17%     
==========================================
  Files          96       96              
  Lines       18753    19579     +826     
==========================================
+ Hits        16834    17609     +775     
- Misses       1919     1970      +51

Impacted Files	Coverage Δ
juniper/src/lib.rs	85.36% <ø> (+3.54%)	⬆️
juniper/src/macros/tests/args.rs	99.57% <ø> (ø)	⬆️
juniper/src/tests/introspection_tests.rs	95.07% <ø> (ø)	⬆️
juniper/src/macros/tests/field.rs	93.9% <100%> (-2.13%)	⬇️
juniper/src/types/base.rs	88.43% <100%> (-1.74%)	⬇️
juniper_tests/src/codegen/derive_object.rs	90.47% <100%> (+3.89%)	⬆️
...r/src/executor_tests/introspection/input_object.rs	94.8% <100%> (+0.27%)	⬆️
juniper/src/executor_tests/introspection/enums.rs	96.24% <100%> (-0.41%)	⬇️
juniper/src/executor_tests/directives.rs	98.95% <100%> (-0.43%)	⬇️
juniper/src/schema/model.rs	86.06% <100%> (+1.66%)	⬆️
... and 35 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56f71e9...af40b11. Read the comment docs.

[dvic]

What is the status of this PR? I believe the impl trait has been removed from this PR, right? Are there any remaining issues/todo's before this can be merged?

[weiznich]

The usage of impl Trait has been already removed in 3b3d041.
This pr is missing a final review/decision from the juniper team.

[LegNeato]

Ah, didn't see this was updated, thanks for the reminder! I'm merging this as-is as I think the change is good and straightforward. I'm slightly concerned about:

1. This is basically implementing a vector-backed arena. It might be better to use one from crates.io, as there are no shortage of them with various perf and mem tradeoffs. Or better yet, have a pluggable interface as @theduke mentioned.

2. Down the road it might not be obvious why we need the arena. I'm worried without some sort of validation or perf test we could regress.

In any case, I don't think those are blockers for landing this.