CDRIVER-5864 Do not parse BSON in mongoc_server_description_new_copy
#1835
Conversation
kevinAlbs
changed the title
mongoc_server_description_new_copymongoc_server_description_new_copy
|
|
||
| #define COPY_BSON_FIELD(FIELD) \ | ||
| if (1) { \ | ||
| bson_init (©->FIELD); \ |
There was a problem hiding this comment.
This init could be omitted, bson_copy_to doesn't require the destination to be initialized.
There was a problem hiding this comment.
Good spot. Updated.
| if (1) { \ | ||
| if (!bson_empty (&description->FIELD)) { \ | ||
| ptrdiff_t offset = bson_get_data (&description->FIELD) - bson_get_data (&description->last_hello_response); \ | ||
| const uint8_t *data = bson_get_data (©->last_hello_response) + offset; \ |
There was a problem hiding this comment.
I know this is a performance patch, but it might still be worth bounds-checking 'offset' here? This might be a good place for a debug-only or asan-only assertion.
There was a problem hiding this comment.
Added debug-only assertions with MONGOC_DEBUG_ASSERT
| #define COPY_INTERNAL_STRING_FIELD(FIELD) \ | ||
| if (1) { \ | ||
| if (description->FIELD) { \ | ||
| ptrdiff_t offset = (char *) description->FIELD - (char *) bson_get_data (&description->last_hello_response); \ |
There was a problem hiding this comment.
This might be a good place for an asan-only assertion too, since it would require a heavy strlen check.
There was a problem hiding this comment.
LGTM.
Since the i64 atomics appears to be a bottleneck, I would assume that this was only showing up on 32-bit builds? On 64-bit systems it shouldn't need to use the emulated atomics. If they are building on 64bit and still getting the emulated atomics, I'd be concerned that our atomic-detection may be broken.
I think the atomics were a red herring. The build was 64-bit. And I expect it was using the atomic intrinsics. Testing a branch with only the atomics removed did not show much improvement. |
#1835) * do not parse BSON in `mongoc_server_description_new_copy` * remove unnecessary atomic modification of `round_trip_time_msec` * add tests for copying
This PR is intended to improve performance of
mongoc_server_description_new_copy. Rather than re-parse the BSON document of thehelloreply each time, fields are copied over individually. Internal references into the BSON document are copied by calculating the offset into the BSON document.Atomic modification to
round_trip_time_msecis removed.round_trip_time_msecappears to only be modified with exclusive access to themongoc_server_description_t(when applying in mongoc_server_description_handle_hello, or modifying the topology description).mongoc_server_description_new_copynow assigns each field in the order declared inmongoc_server_description_tto more easily cross-reference.Verified with this patch build: https://spruce.mongodb.com/version/67938f122a38d700078e6820
Background & Motivation
HELP-69502 notes an observed performance regression from CDRIVER-4114. ded9ae5 trades a lock-and-modify with a copy-modify-and-swap to avoid lock contention on the shared topology description. Neither I nor support has yet been able to reproduce the same slowdown. But a shared flamegraph shows many samples in
mongoc_server_description_new_copy.Running a benchmark calling
mongoc_server_description_new_copy120 times shows promising results:On commit ded9ae5:
On this branch: