Use binary search for find capnp struct field by name

[quartox]

Following discussion in #455 I added members_by_name to RawStructSchema which contains a sorted list of tuples with the name of the field and the index of the field. This is used for binary search when finding the field in a capnp schema using the name of the field copying the C++ binary search algorithm.

I need to test the performance improvement of this change.

[codecov]

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (76590f9) 52.02% compared to head (6ce2871) 51.97%.
Report is 12 commits behind head on master.

Files	Patch %	Lines
capnpc/src/codegen.rs	0.00%	25 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
- Coverage   52.02%   51.97%   -0.05%     
==========================================
  Files          69       69              
  Lines       33954    33996      +42     
==========================================
+ Hits        17664    17671       +7     
- Misses      16290    16325      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[quartox]

Performance testing on this commit cuts time spent in find_field_by_name from 15% to 3.5%. I don't trust the exact numbers but clearly a significant performance improvement. Annoyed that I can't figure out how to do this O(1) but this is a big help.

EDIT: problems with perf were cutting off my flamegraphs. More complete comparison is 43% down to 19%. Was hoping for even better, but that is obviously significant.

[dwrensha]

The direct translation of capnproto-c++ would make this a &[u16]: https://github.com/capnproto/capnproto/blob/537fe97d6ab1962e92e15b8a16f60439a63b90f5/c%2B%2B/src/capnp/raw-schema.h#L184

Is there a reason that you did it this way? The &str values that you are storing here should be cheaply retrievable via StructSchema::get_fields().

[quartox]

Did not know it was cheap to get that, thanks for the tip. I saw some calls to Reader::get_text in the benchmarking flamegraph that seemed expensive, but I think that was only because of the total number of calls to that function (which we already reduced).

Only problem now is that we don't have PartialOrd to compare the field name passed by the user and text::Reader. Previously we only needed exact equality, and now we need less than. Do not know if it is easy to implement this trait if you have any suggestions that would be welcome.

[quartox]

I tested on this branch and using StructSchema::get_fields made it slower than the original O(N) implementation. Most of the time is spent getting the FieldList followed by get_name.

EDIT: improved implementation by saving the FieldList. It is still noticeably slower than implementation in the PR because of the time spent in get_name.
PR version find_field_by_name: 19% of benchmark time
Using fields.get_proto().get_name()? find_field_by_name: 29% of benchmark time

[dwrensha]

In that branch, you are converting to utf8 before making a comparison:

} else if candidate_name.to_str()? < name {

If you directly make the comparison on capnp::text::Reader, how much faster does that make things? (Implementing PartialOrd for text::Reader should be easy, because &[u8] already implements PartialOrd.)

It might also help to add #[inline] attributes on various methods, like StructSchema::get_proto(). (I don't particularly expect this to help, unless it somehow enables cross-crate inlining, but it seems worth a try.)

[quartox]

I added the #[derive(PartialOrd)] trait to the capnp::text::Reader but the less than comparison with &str does not compile. The compiler complains that they are different types. I was surprised since the equal to comparison compiles. For what it is worth the to_str call adds a negligible amount to the flamegraph compared to get_name. I obviously am seeking performance improvement but ideally not adding any unnecessary complexity that makes it more confusing to read.

I will try inlining StructSchema::get_proto. I was also considering an Option field in the StructSchema to cache the FieldList after the first call to StructSchema::get_fields.

[dwrensha]

@quartox roughly how many fields do your structs have?

[quartox]

I was going to try to share the benchmark but it isn't as simple as I had hoped. I was going to construct a schema with similar properties and generate fake data. I haven't had much success using the command line capnp json to binary converter with as much nesting as is happening in my real schemas. I will see if I can find another way to do this.

I will clean up this PR to use members_by_name: &'static [u16] and then we can open another issue to discuss the other performance improvements. Ultimately speeding up the other portions of the code would be more beneficial.

I may explore a static hashmap since the C++ implementation has a todo to make that improvement, but if you want to match what they currently do then we may not adopt it.

[dwrensha]

I'm considering making a version-bump release soon, including #472 as well.

I like the members_by_name: &'static [u16] solution. Can you submit a pull request for that?

[quartox]

I pushed a new commit that switches to the members_by_name: &'static [u16] solution yesterday. Let me know if it looks good. I can make a new PR is that is easier.

[dwrensha]

Ah, I should have noticed that! That should be sufficient. Thanks!