jagill commented on PR #6643: URL: https://github.com/apache/arrow-rs/pull/6643#issuecomment-2551814617
Ok, I've hoisted the conditional outside the loop. It also had the effect (as you predicted) that the gnarly match statement for the delimiters is a bit easier to understand. > 1. Are you planning / willing to implement serializing support too (aka support in json_writer)? That would then allow us to verify that data can be round-tripped I was not planning to, because there was no use-case requiring it. But I'm certainly willing to, both for symmetry and for round-tripping. Is that something you'd want in this PR, or a follow-up? > 2. I believe this PR slows down performance (notes below) When I check, I'm consistently seeing no difference between the main branch and either of the variants: ``` ❯ critcmp main inner-conditional group inner-conditional main ----- ----------------- ---- large_bench_primitive 1.00 2.7±0.01ms ? ?/sec 1.00 2.7±0.03ms ? ?/sec small_bench_list 1.01 8.4±0.09µs ? ?/sec 1.00 8.2±0.06µs ? ?/sec small_bench_primitive 1.01 4.6±0.03µs ? ?/sec 1.00 4.6±0.04µs ? ?/sec ❯ critcmp main outer-conditional group outer-conditional main ----- ----------------- ---- large_bench_primitive 1.00 2.7±0.03ms ? ?/sec 1.00 2.7±0.03ms ? ?/sec small_bench_list 1.00 8.3±0.11µs ? ?/sec 1.00 8.2±0.06µs ? ?/sec small_bench_primitive 1.00 4.6±0.05µs ? ?/sec 1.00 4.6±0.04µs ? ?/sec ❯ critcmp inner-conditional outer-conditional group outer-conditional inner-conditional ----- ----------------- ----------------- large_bench_primitive 1.00 2.7±0.03ms ? ?/sec 1.00 2.7±0.01ms ? ?/sec small_bench_list 1.00 8.3±0.11µs ? ?/sec 1.01 8.4±0.09µs ? ?/sec small_bench_primitive 1.00 4.6±0.05µs ? ?/sec 1.01 4.6±0.03µs ? ?/sec ``` (btw, I didn't know about critcmp; thanks for the pointer!) However, I'm not running in an isolated environment, so the differences I saw in the past seemed to be related to other processes. I'm running on a M2 Mac and haven't tried on with x86 or other architectures, and haven't been able to isolate from the helpful random processes that Macos spins up, so I'll trust your benchmarks. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
