alamb commented on code in PR #12107: URL: https://github.com/apache/arrow-rs/pull/12107#discussion_r4501901353
########## CONTRIBUTING.md: ########## @@ -307,6 +307,8 @@ identifier that can be changed without affecting test semantics. Pull requests that improve performance, especially those that add non-trivial complexity or use `unsafe`, should include evidence of the improvement, such as benchmarks. +A benchmark that improves is not enough on its own. State which part of the change produces the gain, for example by removing it and rerunning. Cover both well-predicted input (constant lengths, all valid) and poorly predicted input (random), because branchless code wins on the second and can lose on the first. Name the inputs that get slower and say whether real workloads are likely to hit them. Review Comment: Actually, it seems that is covered below. Maybe we can consolidate the guidance ########## CONTRIBUTING.md: ########## @@ -307,6 +307,8 @@ identifier that can be changed without affecting test semantics. Pull requests that improve performance, especially those that add non-trivial complexity or use `unsafe`, should include evidence of the improvement, such as benchmarks. +A benchmark that improves is not enough on its own. State which part of the change produces the gain, for example by removing it and rerunning. Cover both well-predicted input (constant lengths, all valid) and poorly predicted input (random), because branchless code wins on the second and can lose on the first. Name the inputs that get slower and say whether real workloads are likely to hit them. Review Comment: Can we also add a note here about submitting the benchmark as a separate PR so we can use our own automation to test it? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
