cadop / dhart

A library for Navigation Graphs, Visibility, Raycasting and other features for Design Humans Analysis RoboTics (DHART).
https://cadop.github.io/dhart/
GNU General Public License v3.0
13 stars 7 forks source link

Add overloads for C# Interface to use flat vector with `unsafe` to speedup passing rays #46

Closed mariuszhermansdorfer closed 1 year ago

mariuszhermansdorfer commented 1 year ago

Reopening the PR adding minor optimizations to the raycasting API:

mariuszhermansdorfer commented 1 year ago

@cadop I'm unable to reopen #41 hence opening a new one here.

cadop commented 1 year ago

Could you elaborate on the workflow for the unsafe use-case?

I understand the purpose of the input being flattened so it doesn't need to be converted from Vector3D to the flat array passed to C.

I am not sure about the unsafe conversion of a Vector3D array though. I imagine you are only using one of them (since they overload the same function), and the float[] method is much faster.

I wrote some performance tests for C# on this branch https://github.com/cadop/dhart/tree/occlusion-performance and it seems like unsafe is slowest. It would be helpful if you could double check in your own situation.

cadop commented 1 year ago

@mariuszhermansdorfer If you are getting similar results when running my test branch, i'll make some changes to your code and write a unittest so we can pull it

mariuszhermansdorfer commented 1 year ago

thanks for the reminder @cadop. I completely forgot about this.

The unsafe part is supposed to be the fastest way of iterating over a list/array. My initial tests confirmed that. Let me re-run the benchmarks and get back to you on this one.

cadop commented 1 year ago

Okay thanks, please also run the branch I mentioned https://github.com/cadop/dhart/tree/occlusion-performance since I had setup the test cases which output the stopwatch times

mariuszhermansdorfer commented 1 year ago

@cadop, I'm back with the results and on my machine the FlattenUnsafe method is consistently faster than the EnumerableFlatten image

image

I am, however, running this on .NET Framework 4.8. That's the only difference to this branch https://github.com/cadop/dhart/tree/occlusion-performance

[EDIT] Results, for different array sizes.

100000000: image image

100000: image image

cadop commented 1 year ago

@mariuszhermansdorfer

Actually this is the same result I was getting. Yes its faster than Enumerable, but FlattenUnsafe is slower than Flatten, which is not using unsafe.

mariuszhermansdorfer commented 1 year ago

I am not sure about the unsafe conversion of a Vector3D array though. I imagine you are only using one of them (since they overload the same function), and the float[] method is much faster.

I think I only understood your question now - you were asking why not always use the method which takes float[] as parameter instead of the overload accepting Vector3D[], correct?

Personally, I'd always work with float[] but since you already had this overload: https://github.com/cadop/dhart/blob/26102d0f38860b8855df3670de68053aa372ae71/src/Csharp/packages/raytracer/src/RayTracerNative.cs#L309-L313

I didn't want to break the API and only suggested a faster internal method to flatten a Vector3d[] if users provide one.

cadop commented 1 year ago

Okay yea I think we are on the same page now. I had wanted to confirm that was the only reason you were doing that.

I think in this case it makes a lot of sense to add the Float method which expects a flat array to the API. With that added, I don't think we should add the unsafe method:

  1. If performance is an issue, users should directly pass a flat array
  2. unsafe is not CLS compliant https://learn.microsoft.com/en-us/dotnet/api/system.clscompliantattribute?view=net-7.0#remarks
mariuszhermansdorfer commented 1 year ago

Gotcha. It sounds like the best option to me as well.

cadop commented 1 year ago

Also I don't actually understand your performance results. For a large array, Flatten is 20ms while Unsafe is 2 seconds? and on a small array it was 26ms?

It seems like either a bug or some really nice compilation optimization with the array.

Any ideas?

mariuszhermansdorfer commented 1 year ago

It's way simpler than that - I only changed the values for the 2 selected tests, ignoring Flatten since it wasn't my focus.

cadop commented 1 year ago

Ah ha!

Could you run with Flatten as well to compare please, just for continuity in this thread.

This was my result for 1,000,000

image

and for 100,000,000

image

mariuszhermansdorfer commented 1 year ago

Sure, results for:

100,000 image

1,000,000 image

10,000,000 image

100,000,000 image

cadop commented 1 year ago

Awesome thank you for checking.

Yea so it seems like Flatten is just faster than using unsafe even though they both take a Vector3D array.

I'll followup tomorrow on this PR. I will probably just pull into dev and then do the docs and changes needed to not burden you. I do appreciate the contributions, and hope to see a grasshopper plugin in the future :)

mariuszhermansdorfer commented 1 year ago

I'll follow up on the GH plugin but am quite swamped with work in the office currently and don't have much time for any side projects...

On another note, did you port any optimizations from OcclusionRays to other methods such as this one as well? https://github.com/cadop/dhart/blob/26102d0f38860b8855df3670de68053aa372ae71/src/Csharp/packages/raytracer/src/RayTracerNative.cs#L249-L254