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

Optimize floatutils #191

Merged

Conversation

yuvaljacoby
Copy link
Collaborator

To validate the improvement I run the following networks:
acas_2_5 property 2 (10 minutes run, SAT), improvement of ~6.8% over 5 experiments
acas_3_3 property 3 (8 minutes run, UNSAT), improvement of ~6.6% over 5 experiments
acas_2_8 property 4 (3 minutes run, UNSAT), improvement of ~7.4% over 5 experiments

Most of the most of the speedup comes from the inline (the size of the executable is similar).
The reduce of operations gives another ~0.5 improvement, in isPositive / isNegiative the compiler can't do this optimization because the epsilon is not static, so that actually removes two operations.
In isZero, we replace a compression into multiply and 2 additions which is supposed to be faster

src/common/FloatUtils.h Outdated Show resolved Hide resolved
@guykatzz
Copy link
Collaborator

guykatzz commented Oct 16, 2019

Changes look okay. IsNegative and IsPositive definitely seem better; regarding isZero, I don't know, so an extensive comparison is needed. Can you start a cluster experiment on the entire ACAS Xu benchmark suite? At least properties 1-4.

Also, it's best to isolate the changes - maybe a subset is better.

@yuvaljacoby
Copy link
Collaborator Author

I did the test separate, checked only the inline and then inline + the code changes (all code changes together).
most of the performance come from the inline, and a bit more from the code changes.

BTW, in gcc5 (and 7) it's enough to move the implementations to the .h file and the compiler is making the function inline automatically (so we can remove the inline keyword, a matter of style)
didn't check clang

@ahmed-irfan
Copy link
Collaborator

@yuvaljacoby why not put the full implementation of floatutils in the header file then....

@guykatzz
Copy link
Collaborator

Hi Yuval,
What's the status of this pull request?

@yuvaljacoby
Copy link
Collaborator Author

I am sorry, forgot about it.
Will get back to it in the next couple of days

@ahmed-irfan
Copy link
Collaborator

@yuvaljacoby here is the comparison you asked for:

p4.txt
p3.txt
p2.txt
p1.txt

Copy link
Collaborator

@ahmed-irfan ahmed-irfan left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks

@yuvaljacoby
Copy link
Collaborator Author

Summering for future reference (only avg cpu):
p1:
original: 140421.1
inline: 139222.3
p2:
original: 63249.9
inline: 61641.4
p3:
original: 23690.4
inline: 22135.7
p4:
original: 10303.6
inline: 9252.6

@guykatzz
Copy link
Collaborator

guykatzz commented Apr 6, 2020

LGTM

@yuvaljacoby yuvaljacoby merged commit 7422477 into NeuralNetworkVerification:master Apr 6, 2020
@yuvaljacoby yuvaljacoby deleted the optimize_floatutils branch June 14, 2020 07:58
AleksandarZeljic pushed a commit to AleksandarZeljic/Marabou that referenced this pull request Oct 9, 2020
Move most of FloatUtils implementation to .h (to enable compiler inline optimization)
AVG CPU results (on acas xu properties) 
p1:
original: 140421.1
inline: 139222.3
p2:
original: 63249.9
inline: 61641.4
p3:
original: 23690.4
inline: 22135.7
p4:
original: 10303.6
inline: 9252.6
matanost pushed a commit that referenced this pull request Nov 2, 2021
Move most of FloatUtils implementation to .h (to enable compiler inline optimization)
AVG CPU results (on acas xu properties) 
p1:
original: 140421.1
inline: 139222.3
p2:
original: 63249.9
inline: 61641.4
p3:
original: 23690.4
inline: 22135.7
p4:
original: 10303.6
inline: 9252.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants