[SERVER-81191] Checking array field names in IDL is expensive Created: 19/Sep/23 Updated: 09/Nov/23 Resolved: 06/Nov/23 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 7.2.0-rc0 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Matthew Russotto | Assignee: | Patrick Freed |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | perf-tiger, perf-tiger-handoff, perf-tiger-triaged | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Assigned Teams: |
Service Arch
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Sprint: | Service Arch 2023-10-02, Service Arch 2023-10-16, Service Arch 2023-10-30, Service Arch 2023-11-13 | ||||||||
| Participants: | |||||||||
| Description |
|
When we parse a BSON array in IDL, we check that all the field names are as expected using the general purpose NumberParser class, and for larger arrays this can take significant time. For an example, here is the parsing of an oplog entry using code from
(The parameter is the number of entries in the statementID array; the first two benchmarks are not using arrays.) Here is the same benchmark with all array fieldname checking removed
And here is the code using the C++ "std::from_chars" method
I tried a few other things like encoding the expected field number and comparing that, and incrementing the expected field number represented as a string; they weren't faster than from_chars. These timings are on my Intel workstation so not too precise, but the differences are significant. Ideally we wouldn't even have field names in BSON arrays but I think that ship has long since sailed. |
| Comments |
| Comment by Githook User [ 03/Nov/23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Patrick Freed', 'email': 'patrick.freed@mongodb.com', 'username': 'patrickfreed'}Message: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Patrick Freed [ 05/Oct/23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I reran the benchmarks on my Intel machine, this time with gcc, and I am seeing similar results now, sorry for that oversight. I'm curious why there's such a discrepancy here, I was under the impression that we used the same libstdc++ on both gcc and clang, except if the --libc++ flag were passed to the scons invocation. Given that, I would've expected these to be closer, but I guess gcc just has some better optimization for this particular path. Given that it's relatively close when built with clang and dramatically faster when built with gcc, it seems prudent to go with from_chars after all then, at least for integers. The release binaries are built with gcc, so that's what matters more anyways. I'll revisit the actual implementation tomorrow. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Matthew Russotto [ 04/Oct/23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Running your benchmark on an ARM virtual workstation I get
which I believe roughly matches what I got before with from_chars being a LOT faster for small integers. It's also dramatically faster for full range integers:
This is with compile options taken from the perf project build. With the --opt build style (which is a clang build, not GCC), I get different results:
NumberParser is worse than on gcc, but from_chars is dramatically worse. Both of these should be using the v4 toolchain (though one is clang andthe other is gcc) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Patrick Freed [ 04/Oct/23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
So I looked into this a bit more, and it's looking like from_chars isn't a guaranteed win for general purpose number parsing unfortunately, at least not in the version provided in the v4 toolchain's libstdc++. I ran a simple benchmark which converted strings to integers (randomly generated from within the min/max bounds of int), and it actually looks like the existing parser is faster for that use case.
I was able to partially reproduce the results in the ticket description if I changed the benchmark to parsing the string versions of 0 to n (similar to array indices) though:
I don't know why this is the case, but I guess from_chars is is better for converting smaller strings/numbers than our existing implementation is. One thing I noticed is that more recent versions of libstdc++ include performance optimizations for from_chars, which might explain what we're seeing here: https://github.com/gcc-mirror/gcc/commit/a54137c88061c7495728fc6b8dfd0474e812b2cb#diff-8ae63de6891112977cbd61ab5c02264e1dd6dc079ad717416ba03e689bbd1c81 Per the tags on that commit, it looks like we'll need gcc 12.1 to take advantage of them, though, and the v4 toolchain is currently on 11.3. In light of all this, I think what I would suggest is that rather than updating NumberParser's implementation to use from_chars, we should consider maybe changing the implementation of our BSON array parsing in particular to use from_chars, since its a code path that from_chars is better at. We could also consider just not validating array indexes here at all--there's prior art for this in the BSON corpus driver specification: drivers are intended to be able to parse invalid array keys (https://github.com/mongodb/specifications/blob/master/source/bson-corpus/bson-corpus.rst#testing-validity). If we're okay with the latter, then that would probably be the easiest and fastest. In the future, we can revisit updating the NumberParser implementation again to see if newer libstdc++ versions yield better results. benchmark code: https://github.com/patrickfreed/mongo/blob/parse-number-bm-old/src/mongo/base/parse_number_bm.cpp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Patrick Freed [ 20/Sep/23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This is closely related to matthew.russotto@mongodb.com Can you share the benchmark code you were using to produce these numbers? I want to make sure my updated version matches what you're seeing. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 19/Sep/23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Really interesting. I'm surprised std::from_chars is so much faster than NumberParser, which predates it. As an aside, perhaps NumberParser should lean on the new no-locale C++ numeric parse routines more. I guess the heart of the ticket is whether it is safe to ignore BSON array indices? Seems like it should be ok in practice, but it's kind of a subjective question. Using std::from_chars approaches the performance of removing the checks. Our NumberParser uses an internal parseMagnitudeFromStringWithBase, which is doing overflow protection during the parse. Unnecessary in this case, and we should probably have an option to deselect it. We might be suffering performance from returning a StatusWith as well. Exceptions would be a better choice here since array indices are presumably reliably formatted to be valid integers. |