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

Fix perf test #575

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Fix perf test #575

merged 1 commit into from
Oct 10, 2023

Conversation

sloriot
Copy link
Contributor

@sloriot sloriot commented Oct 10, 2023

the runtime and output are now similar for CGAL and Manifold (tested with one thread).

@google-cla
Copy link

google-cla bot commented Oct 10, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@elalish
Copy link
Owner

elalish commented Oct 10, 2023

Can you give some before & after numbers to quantify? It would also be helpful to see the multi-threaded comparison.

@ochafik Would you like to review this? I'm not sure why these changes would make much difference.

@pca006132
Copy link
Collaborator

I think this is written before the lazy csg pr? Previously translate mutates the original object, but now it does not.

But I don't see anything that changes the performance...

@pca006132
Copy link
Collaborator

pca006132 commented Oct 10, 2023

OK I got some numbers:

$ time taskset --cpu-list 0 ./extras/perfTestCGAL
nTri = 512, time = 0.0025262 sec
nTri = 2048, time = 0.00482419 sec
nTri = 8192, time = 0.0144991 sec
nTri = 32768, time = 0.0448983 sec
nTri = 131072, time = 0.181884 sec
nTri = 524288, time = 0.804409 sec
nTri = 2097152, time = 3.77978 sec
nTri = 8388608, time = 17.4572 sec
taskset --cpu-list 0 ./extras/perfTestCGAL  27.38s user 3.45s system 99% cpu 30.833 total

$ time taskset --cpu-list 0 ./extras/perfTest
nTri = 512, time = 0.00125314 sec
nTri = 2048, time = 0.00342612 sec
nTri = 8192, time = 0.0118149 sec
nTri = 32768, time = 0.0443256 sec
nTri = 131072, time = 0.179852 sec
nTri = 524288, time = 0.754306 sec
nTri = 2097152, time = 3.27157 sec
nTri = 8388608, time = 14.0492 sec
taskset --cpu-list 0 ./extras/perfTest  19.12s user 3.19s system 99% cpu 22.311 total

$ time ./extras/perfTest                     
nTri = 512, time = 0.00152849 sec
nTri = 2048, time = 0.00311421 sec
nTri = 8192, time = 0.00468721 sec
nTri = 32768, time = 0.00929102 sec
nTri = 131072, time = 0.0311873 sec
nTri = 524288, time = 0.102621 sec
nTri = 2097152, time = 0.487056 sec
nTri = 8388608, time = 2.15144 sec
./extras/perfTest  52.75s user 7.71s system 1588% cpu 3.806 total

note that some overhead is spent on converting manifold sphere to mesh and import into cgal.

@pca006132
Copy link
Collaborator

so I guess this is good, and we just need to wait for CLA?

@sloriot
Copy link
Contributor Author

sloriot commented Oct 10, 2023

I think this is written before the lazy csg pr? Previously translate mutates the original object, but now it does not.

But I don't see anything that changes the performance...

The benches between manifold and CGAL were not doing the same thing so comparing runtimes was meaningless. Now they are doing the same thing.

@pca006132
Copy link
Collaborator

yes thanks a lot, we were unaware of this because we changed the API a year ago and did not run this benchmark for a long time.

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

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

Thanks! Any trouble signing the CLA? Generally the individual one is much easier than the corporate one, if that's helpful.

@sloriot
Copy link
Contributor Author

sloriot commented Oct 10, 2023

Thanks! Any trouble signing the CLA? Generally the individual one is much easier than the corporate one, if that's helpful.

It's signed but does not show up yet.

@pca006132 pca006132 merged commit 062399c into elalish:master Oct 10, 2023
16 checks passed
@sloriot sloriot deleted the fix_cgal_perf branch October 10, 2023 18:11
@elalish elalish mentioned this pull request Nov 3, 2023
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
do translate the sphere
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