-
Notifications
You must be signed in to change notification settings - Fork 84
IntDomain code optimizations #943
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
IntDomain code optimizations #943
Conversation
|
Here are the three instructions that we used to run Goblint for this PR so far: |
|
Note: We decided to go for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this! I left some remark explaining why the issue with the failing tests is occurring.
src/cdomains/intDomain.ml
Outdated
|
|
||
| let cast_to ?torg ?no_ov t = norm ~cast:true t (* norm does all overflow handling *) | ||
|
|
||
| let interval_threshold_widening_ref = ref @@ get_bool "ana.int.interval_threshold_widening" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit tricky here: This reference is set when the IntervalFunctor is instantiated, which is right at the start of the program, when the configuration is not yet read. Since the passed configuration is not read yet, the value that is passed for this configuration option cannot be retrieved at that point in time. This also should explain the failing test case. The same holds true for the other references.
An option would be to change this constant into a function that takes unit, and reads the configuration option when the function is first called, and later retrieves this stored value. Another alternative would be make the evaluation of the value lazy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip, make test now passes all tests after adding lazy evaluation of the bool value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lazy will misbehave in server mode, because the option will only be read once. Our ResettableLazy would work around that, but if we really want to optimize this all the way, mutable variables would have the least overhead. The only thing is, they have to be initialized from options in a function, which is called after the options have been handled and reloaded in the server mode.
Since there are multiple of these cached options, instead of a bunch of refs, it's more convenient to use a record with mutable fields, which also avoids the additional indirection of a ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also tried to do it with function that takes unit as mentioned above and also make test passes all tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lazywill misbehave in server mode, because the option will only be read once. OurResettableLazywould work around that, but if we really want to optimize this all the way, mutable variables would have the least overhead. The only thing is, they have to be initialized from options in a function, which is called after the options have been handled and reloaded in the server mode.Since there are multiple of these cached options, instead of a bunch of
refs, it's more convenient to use a record withmutablefields, which also avoids the additional indirection of aref.
Thanks for the feedback! We'll make sure to look into the solution with a record in place of a ref. Currently we have a solution with a ref variable which passes make test and has the best running time out of all attempts. Hence, we now have to optimize this with a record, as you suggested.
This is probably because the option to use the |
|
@jerhard Thanks a lot for the input and the feedback! We'll look into the options that you suggested :) |
|
We refactored the mutable variable way of reading config options and moved it into its own module in the |
src/analyses/base.ml
Outdated
| ) st sets | ||
| ) | ||
| |> List.of_seq | ||
| |> BatList.fold_left D.meet st |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could one just use the fold_left on Seq here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip, I tried it and it seems to work fine, so I will try to run Goblint now and see if we get any improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seq.map and Seq.fold_left are fine, but one has to be careful using Seq functions, because many were just added in OCaml 4.14, while we also support 4.10: https://v2.ocaml.org/api/Seq.html.
|
I changed the current config value reading optimization as follows:
and the running time was still not very impressive, namely Due to this result, we decided to try and slim down the config optimization code a bit and avoid the Please let us know in case you also have any remarks |
This brought us again in the ball-park of ~1456s of |
|
Also tried to put all the |
|
Regarding the difference between using On my personal laptop the following benchmarks were observed:With flambda:
Without flambda:
On the cluster the following benchmarks were observed:With flambda:
Without flambda:
|
|
According to my last comment (#943 (comment)), I'm not sure if changing the way that configs are read is really necessary, considering the fact that the speed-up is not so dramatic. |
I didn't have a look at the current state of this PR yet, but just an interesting tidbit: the compilation of matches over constant strings is highly efficient in OCaml: https://discuss.ocaml.org/t/constant-string-pattern-matching/10801. |
sim642
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be rebased onto the current master after #928.
|
Please rebase this PR, it is currently very hard to review since it includes both changes due to flambda, Seq, and the actual optimization. |
sim642
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there no need to cache the int domain activation options like ana.int.interval? I recall from profiling a while ago that those also were noticeable.
I tried some further optimization for the activation options of the integer domains in the Below I'll post some runs that I did locally on my machine which show some speed-up for the
|
|
The I suspect somewhere something is done with the int domains before the config is read, so the default activations are cached and any options activating more int domains are ignored. Maybe some debug printing/exception raising in those functions which do the caching reveal when it happens. |
This broke int domain activation caching in 7c2455a.
I fixed this by adding a corresponding
The problem was caused by |
sim642
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR history contains some old flambda stuff and lots of back-and-forth, so a squash merge would be appropriate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I ran some further benchmarks on this branch, compared to the current latest state of the On
|
This PR tackles some code optimization attempts in the
intDomain.mlfile. Namely:get_boolaccesses to configuration flags of Goblint which can be stored in variables and looked up instead of being evaluated every timeget_boolare:ana.int.interval_threshold_wideningana.int.interval_narrow_by_meetana.int.def_exc_widen_by_joinWhat we currently do as a solution is to store all 3
get_boolcalls to mutablerefvariables and just dereference said variables and use them in place of the original config lookups.Note:
bench/coreutils/ls_comb.cas inputflambda-O3flagFor
ana.int.interval_threshold_widening, the results are as follows (with the flags:--enable ana.int.intervaland--enable ana.int.interval_threshold_widening):11.858sofwalltimemake test, the two tests which include the same set of flags for Goblint (namely03/24and58/17) failed. Hence, some semantics might be broken by this change. We decided to keep it for now anyways and ask why the tests are failing exactlyFor
ana.int.interval_narrow_by_meet, the results are as follows (with the flags:--enable ana.int.intervaland--enable ana.int.interval_narrow_by_meet):11.908sofwalltimeFor
ana.int.def_exc_widen_by_join, the results are more or less the same with and without code changes (roughly 12 seconds and with the flags--enable ana.int.def_excand--enable ana.int.def_exc_widen_by_join):--enable ana.int.def_excand--enable ana.int.def_exc_widen_by_join) which ran successfully both before and after the code changes heremake testran successfully here both prior to and after the code changesmake testrun)We would like to discuss the stated changes of the PR and see if they're viable and make sense.