Closed tgiannak closed 1 year ago
@nmacedo did we take care of this issue in the change to 6.0?
Seems instantaneous on 6.0.
openjdk version "11.0.18" 2023-01-17
OpenJDK Runtime Environment (build 11.0.18+10-post-Ubuntu-0ubuntu122.04)
OpenJDK 64-Bit Server VM (build 11.0.18+10-post-Ubuntu-0ubuntu122.04, mixed mode, sharing)
@tgiannak please reopen if this issue still exists in 6.x
Alloy version: 5.1.0 Java version:
I've run into a performance bug in
ensureDef
. Unfortunately, I can't share the actual model that led me to discover the problem, but I was able to create the following synthetic model that is much smaller and has the same issue.The model below takes approximately 200 seconds to translate to CNF with "Prevent overflows" on and Skolem depth 0. With "Prevent overflows" turned off and Skolem depth 0, it takes less than 5 seconds. When running with "Prevent overflows" turned on, more than 90% of the time is spent in calls to
toString
within calls toensureDef
. When there are larger expressions in the quantifier bindings, the discrepancy is even more dramatic and an even larger percent of the time is spent intoString
. (I had one run that took nearly 10 minutes with "Prevent overflows" on but less than one second with it off.)From reading the code, I could identify three causes of the poor performance:
The
isInt
method is callingtoString
on non-trivial data structures. This is both slow and causes a lot of garbage to be produced.This might be improved by changing the
!(expression instanceof Expression)
condition to!(expression instanceof ConstantExpression)
.ensureDef
repeats the expensiveisInt
call for every element ofenv
for every element ofdcs
, rather than filteringenv
once, up-front.ensureDef
is called redundantly betweeneq
andsubset
.Changing the conditional is probably the easiest way to get a big performance win for
ensureDef
in situations where there are nested quantifiers over non-trivial expressions, since it would limit the calls totoString
to a cheap case (with noString
concatenation) without affecting the result of theisInt
call. All three issues probably have to be addressed for a true fix.(Sorry for the oddness of the model. It took some work to get the rest of Alloy and Kodkod not to optimize everything away before reaching the problematic code.)