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

Scalar field improvements #103

Merged
merged 2 commits into from
May 30, 2024
Merged

Scalar field improvements #103

merged 2 commits into from
May 30, 2024

Conversation

tpwrules
Copy link
Contributor

@tpwrules tpwrules commented May 4, 2024

Please see commits for details.

tpwrules added 2 commits May 10, 2024 10:17
Opens opportunity to e.g. compute lazily.
Use local variable for performance (compiler can't optimize store due to
exception possibility). Reduces execution time approximately 30%.

Also properly clears min and max if the field is not empty, but all
points are invalid.
@tpwrules
Copy link
Contributor Author

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

@dgirardeau
Copy link
Member

Ah, I forgot about this PR sorry.

Is it only about syntax?

@tpwrules
Copy link
Contributor Author

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

@dgirardeau
Copy link
Member

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

@tpwrules
Copy link
Contributor Author

tpwrules commented May 30, 2024

  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
Copy link
Member

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

@dgirardeau
Copy link
Member

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

@tpwrules
Copy link
Contributor Author

tpwrules commented May 30, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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.

@dgirardeau dgirardeau merged commit 2f1cfcf into CloudCompare:master May 30, 2024
8 checks passed
@tpwrules tpwrules deleted the scalar-field branch May 30, 2024 16:45
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