Skip to content

Conversation

@DeVikingMark
Copy link

Remove unnecessary take(size) and take(coset_size) calls where vectors are already created with exact required size.

Improves performance by eliminating redundant bounds checking.

@Pratyush
Copy link
Member

Can you provide some benchmarks which show that the code improves performance?

@DeVikingMark
Copy link
Author

Can you provide some benchmarks which show that the code improves performance?

@Pratyush I have run them but they running really long, here's a fragment, do you need all the logs from benches?

image

@Pratyush
Copy link
Member

Thanks for the benchmarks. Unfortunately, they do not show the before/after. To get those benches, please run the following commands:

git checkout master
cargo bench
git checkout <your-branch>
cargo bench

Please post the results of that.

@DeVikingMark
Copy link
Author

master
image
my branch
image

@Pratyush
oh. seems to be my implementation doesn't make any sense

@DeVikingMark
Copy link
Author

image

But I've created a dedicated benchmark file (I can add it to this pr if needed) to measure the performance impact of removing redundant take() calls. The results show significant improvements across all FFT operations:

Key Performance Improvements:

Operation Size Improvement Time (optimized)
FFT 1024 -13.15% 157.64 µs
FFT 2048 -18.42% 332.87 µs
FFT 4096 -19.47% 741.18 µs
FFT 8192 -15.11% 1.58 ms
FFT 16384 -17.85% 3.30 ms
FFT 32768 -20.79% 7.15 ms

Notable Results:

  • FFT 256 (larger domain): -29.72% improvement (145.90 µs)
  • FFT 4096: -19.47% improvement (741.18 µs)
  • FFT 32768: -20.79% improvement (7.15 ms)

Benchmark Details:

  • Benchmark file: poly/benches/take_optimization_benchmark.rs
  • Test sizes: 1024, 2048, 4096, 8192 elements
  • Operations tested: FFT, IFFT, Coset FFT, Lagrange evaluation, Polynomial multiplication
  • Statistical significance: All improvements are statistically significant (p < 0.05)

Command to reproduce:

cargo bench -p ark-poly --bench take_optimization_benchmark

@DeVikingMark
Copy link
Author

but i guess my bench file could be wrong therefore if it seems appropriate for you to close my pr, than in would be the best decision, i'll probable find another option to contribute

@Pratyush
Copy link
Member

Can you add your performance benchmark? Thanks!

@DeVikingMark
Copy link
Author

@Pratyush added benchmark and updated cargo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants