descriptor: resolve each stored field once in DynamicMessage::for_each_set - #439
Open
serhiy-bzhezytskyy wants to merge 1 commit into
Open
Conversation
…h_set The loop resolved the field by number, then called `has`, which looked the same number up in the field map again to apply the presence rule to a value the loop already held. Apply the rule to that value through one helper shared with `has`. Per set field: 10.3–11.3 ns → 6.2–6.5 ns (AnalyticsEvent), 16.4–17.5 → 8.9–10.5 (GoogleMessage1). Fixes anthropics#432 item 1; the borrowing iterator in item 2 is a separate API addition.
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Item 1 of #432.
What changed
for_each_setapplies the presence rule to the value it is already holding instead of callinghas(fd), which looked the same field number up in the field map a second time (buffa-descriptor/src/reflect/dynamic.rs:1650-1663on 053fcf3; in debug buildshasalso re-resolved the descriptor for its membership assert).is_present(fd, value), used by bothhasandfor_each_set; the comment that explained it moved with it.ValueRefs handed to the callback are unchanged:haslooked upfd.number(), andfield_or_extension(number)returns the descriptor with exactly that number for declared fields and extensions alike, so it found the same value the loop holds.Why
Per stored field the loop did a
BTreeMapstep, a descriptor lookup by number, and then a secondBTreeMaplookup insidehasfor a value it already had. Measured on the benchmark datasets,for_each_setalone over pre-decoded messages, release build, 2,000 iterations × 3 runs, same binary onmainand on this branch:AnalyticsEvent(50 messages)GoogleMessage1The existing
reflect/*/dynamic_read_allcriterion benches include the decode and move by less than their noise, so the isolated timing is the number. The probe is not part of this change.Item 2 of the issue — a borrowing
set_fields()iterator beside the trait method — is an API addition and is left for a separate change once its shape is decided.Tests
dynamic_message_for_each_set_agrees_with_has: an implicit-presence scalar at its default (skipped) and non-default, anoptionalfield at its default (visited), and a message field; asserts the visited set equals the set of fields for whichhas()is true, and equals[14, 16, 17].cargo test -p buffa-descriptor --features reflect— 199 passcargo test -p buffa-test— 564 passChecks
cargo clippy --workspace --all-targets -- -D warningscargo fmt --all --check.changes/unreleased/Written with Claude Code.