Speed up map-to-struct field fallback matching#789
Conversation
fxamacker
left a comment
There was a problem hiding this comment.
@trevorprater, thanks for opening this PR and including benchstat results.
When case-sensitive match (fastest path) fails, I agree it is a good idea to use an inlinable ASCII fast path before falling back to strings.EqualFold.
There is also an alternative approach worth considering.
Per CONTRIBUTING.md, the issue-first step is meant to align on approach before the implementation work.
This PR is missing the PR template/checklist that is automatically added to all opened PRs.
I found the candidate while experimenting with Sleepy - https://sleepy.run/mcp, then manually reviewed and validated it.
The PR template asks each contributor to sign off on the Developer Certificate of Origin (DCO) certifying that you created the contribution and have the right to submit it under this project's license. Since AI-generated code has unclear provenance (and can reproduce licensed code), that certification is hard to stand behind for AI-written code. So I'd like the implementation to be written by a person.
I'll close this PR shortly, since it's missing the template/DCO and the commits are missing the required Signed-off-by: line.
If you'd like to implement it, please feel free to open an issue about speeding up the fallback to case-insensitive field name matching and we can align on the approach there. One thing to weigh: this optimizes the case-insensitive fallback, which most decoding doesn't hit, so it's worth deciding whether it's worth your effort.
Thanks!
|
Thanks for the detailed feedback. That all makes sense. I agree this PR is not the right thing to spend maintainer time on: the fallback-to-case-insensitive path is not a common decode path, and I am only interested in pursuing changes here if they are clearly impactful and worth reviewing. I did some further local research on the common Given your process guidance, I am happy to either close this PR or leave it for you to close. If you are open to it, I can first open an issue describing the higher-impact decode-path opportunity and the benchmark evidence, then align on scope before writing any implementation. Any future contribution would be written/reworked by me, use the PR template, and include the required DCO Thanks again, and sorry for skipping the issue-first/template/DCO process here. |
|
For rough context on the higher-impact direction I mentioned, I put the research prototype diff here: https://gist.github.com/trevorprater/bc3b57e6193b10f27917489152d6e8e2 This is intentionally not a proposed patch: it is broad, generated during local optimization research, and would need to be split/reworked into small human-authored changes after issue-first discussion. I am sharing it only to show the kind of common decode-path improvements I am looking at, not to ask you to review this diff as-is. |
Summary
This speeds up the case-insensitive fallback path used by map-to-struct decoding.
The exact field-name map lookup still runs first. When fallback matching is enabled, the new helper compares the struct field name with the raw key bytes on an allocation-free ASCII path and only converts the key bytes to a string if Unicode
strings.EqualFoldbehavior is actually needed.Results
Run on Apple M4:
Selected
benchstatresults:B/opandallocs/opwere unchanged across the benchmark matrix.Correctness
The exact match path and integer-key handling are unchanged. The new ASCII helper returns to
strings.EqualFoldwhenever either input contains non-ASCII bytes, preserving the previous Unicode case-folding behavior.I also checked the new helper against the previous
strings.EqualFoldimplementation with fixed ASCII/Unicode cases and 10,000 deterministic random byte-string cases before dropping the temporary oracle test from this PR.Validation:
go test -count=1 ./...I found the candidate while experimenting with Sleepy - https://sleepy.run/mcp, then manually reviewed and validated it.