search

CloudCompare / CCCoreLib

C++ library which provides data structures & algorithms for working with 3D point cloud data
Other
151 stars 50 forks source link

Scalar field improvements #103

Closed tpwrules closed 3 months ago

tpwrules commented 4 months ago

Please see commits for details.

tpwrules commented 3 months ago

@dgirardeau Hello, would like to make sure this gets reviewed.

dgirardeau commented 3 months ago

Ah, I forgot about this PR sorry.

Is it only about syntax?

tpwrules commented 3 months ago

No, there is a little bit of a behavior change. Please see the commit messages.

dgirardeau commented 3 months ago

Sorry I fail to see what's optimized / improved 😅

tpwrules commented 3 months ago
  1. The first commit allows us to change to lazily calculate m_minValue and m_maxValue in the future by making sure it is only accessed through getMin() and getMax(). I don't yet know if this is worthwhile.
  2. The second commit changes the loop to update a local copy of minVal and maxVal and then assign them to the member variables after the loop. This avoids the need to update the member copies and improves performance by about 30%. I noticed that computing min and max is an operation where a lot of time is spent in my tests so this is a worthwhile improvement.
  3. As a side effect of the above, the logic is rearranged so that the member copies are always updated. Previously, the member variables would not be updated properly if there were points in the cloud, but all were the invalid value; the member variables would keep their previous value. Now in this case the min and max are set to 0. This is the behavior change.
dgirardeau commented 3 months ago

On which OS have you performed your benchmarks? Not sure to grasp why updating the members would be slower 🤔

dgirardeau commented 3 months ago

And just to be sure again, was it in Release or Debug mode?

tpwrules commented 3 months ago

I checked on x86_64 Linux in Release mode. The compiler has to keep reading and writing the member variables in memory each loop iteration so that they have the latest value, I'm assuming in case of an exception. Usually this is not such a big deal but for how short the loop is it turns into a measurable overhead. If locals are used then they can be kept in registers. I checked the assembly code.

dgirardeau commented 3 months ago

Wow, that's good to know. And I guess it must be the same on Windows if that's the explanation ... I guess there are other places where this happens then.

tpwrules commented 3 months ago

I did also remove the case through the function where the member variables never get set, maybe having that is why the compiler decided to access memory each iteration. I'm not exactly sure to be honest.