Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 100k/1m uniform random benchmarks #8

Merged
merged 2 commits into from
Sep 18, 2018

Conversation

mourner
Copy link
Contributor

@mourner mourner commented Sep 14, 2018

@delfrrr Added some more benchmarks (100k and 1m random) in this PR.

Roughly 6 times faster then JS version

I was curious about this statement and loos like it's misleading — benchmarks show only about ~20-30% improvement, not 6x. And I get similar perf numbers to the C++ version in my Rust port https://github.com/mourner/delaunator-rs

Copy link
Collaborator

@cleeus cleeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest a benchmark::DoNotOptimize(delaunator) statement in the benchmark loops.
I know, it's unlikely that the optimizer is clever enough to figure out that it can throw away the whole code, but then again ... it's header only and compilers are only getting better.

@delfrrr delfrrr merged commit f61a3ae into delfrrr:master Sep 18, 2018
@mourner mourner deleted the rand-benchmarks branch September 18, 2018 07:35
@mourner
Copy link
Contributor Author

mourner commented Sep 18, 2018

@delfrrr can you also fix the misleading statement in the readme about 6x faster than JS, or at least explain how you got this result?

@delfrrr
Copy link
Owner

delfrrr commented Sep 18, 2018

@mourner will do soon, need to dig into results;
I made current statements based on results from this script https://github.com/delfrrr/delaunator-cpp/blob/master/generate-reference-triangles/index.js

I agree that they may be not precise; also note the version I was testing against;

@mourner
Copy link
Contributor Author

mourner commented Sep 18, 2018

@delfrrr the script doesn't have any warmup, which is especially important for JS because it's JIT-optimized. It might have also measured a dataset that's too small.

@delfrrr
Copy link
Owner

delfrrr commented Sep 19, 2018

I modified cpp and js benchmark to make it more similar;
for JS benchmark I removed part where points are transformed to typed array, so results are even slightly better for JS. Also added more samples for CPP

plot 39

I think there is ~30ms overhead in JS version which makes much slower on small number of points. On 1M points difference is ~10%; since time is not growing linear with number of points comparison in percents is not relevant.

@mourner btw, what is estimated complexity of algorithm itself?

@mourner
Copy link
Contributor Author

mourner commented Sep 19, 2018

@delfrrr it should be O(n log n) in average. The 30ms JS "overhead" is likely just the JIT warmup — subsequent runs on the same amount of points should be much faster.

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.

3 participants