-
Notifications
You must be signed in to change notification settings - Fork 46
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
Replace Z3 phantom reference map by doubly-linked list. #355
Conversation
Really? The doubly-linked list has 2n references + n The hash map consists of one array with a size of 2n to 4n references (references for key and value, with overhead for empty cells), so 128 to 256 bits per entry. The hash map also references So if I did not overlook something, the list is not strictly smaller than the map, if the latter happens to be >= 75% full and the But I see a major additional argument: The memory consumption of the list shrinks again once fewer elements exist. Of course, having add/remove be guaranteed O(1) instead of just amortized is also nice. |
|
||
// Force clean all ASTs, even those which were not GC'd yet. | ||
// Is a no-op if phantom reference handling is not enabled. | ||
for (long ast : referenceMap.values()) { | ||
Native.decRef(getEnv(), ast); | ||
} |
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.
What is the reason for deleting this?
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 was by accident because I misinterpreted the purpose. I added it back.
However, I noticed that if the PhantomReferences
are already enqueued, then they will never get removed from the queue after closing (until the context object itself gets collected).
Unfortunately, ReferenceQueue.clear
does not exist and one has to poll all entries to clear them.
Should I add this or does this not really matter?
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.
You mean those references that have been enqueued before close()
is called? These could in principle be cleaned up, but note that there will likely also be references that are enqueued after close()
is called, and there is nothing we can do about them, so I do not think we need to bother with the already enqueued ones.
Once the solver context becomes unreachable and is cleaned up by GC, the queue and all PhantomReference
s are also cleaned up. So this affects only users who close the context but then keep a reference to it for a longer time.
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.
See my answer to the bulk deletion point you raised further below.
Added back force deletion of native objects.
I was expecting this response :).
It is true that I accidentally calculated in the boxing of the longs, but those do already exist in all cases where the function is used. Nevertheless, the load factor of
Good point. This also means the upper bound
Yes, it performs A LOT better in extreme cases where the hashmap's operations start to approach linear time. |
Co-authored-by: Philipp Wendler <2545335+PhilippWendler@users.noreply.github.com>
I noticed that the |
… comparison. Z3 uses internal formula hashing, thus there should not be multiple pointers to the same boolean constants. This reduces the number of JNI calls for some simple cases.
Build-dependencies produce the largest artifact, around 160 MB for JavaSMT. We do not need those artifacts being around for longer. The latest artifact of a repository is kept for longer than this expiration time, so this change should not affect the reuse of any build-cache.
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 could not detect a breaking example or unit test. The code itself looks valid.
However, I am not sure whether we need/want to merge this PR.
Executing a command like
java -cp bin:lib/java/runtime-z3/common.jar:lib/java/runtime-z3/com.microsoft.z3.jar org.sosy_lab.java_smt.example.nqueens_user_propagator.NQueens 11 2
or
java -cp bin:lib/java/runtime-z3/common.jar:lib/java/runtime-z3/com.microsoft.z3.jar org.sosy_lab.java_smt.example.nqueens_user_propagator.NQueens 11 3
showed a no speedup, e.g., the example runs for about 6.0-6.5s, with and without this change.
Perhaps the speedup was related to other optimizations implemented for Z3 or user propagation.
Have you enabled phantom references (they are disabled by default)? (*) The robustness is only for formulas reported by the user propagator itself. If the user decides to create new formulas from inside the callbacks, they can still end up with millions of phantom references and a possibility of encountering bad map performance. EDIT: Btw. on what hardware are you running the Nqueens example? My machine runs 11-Queens in 2 seconds rather than 60. I'm surprised by such a big discrepancy. |
First: I forgot to enable phantom references. Thanks for the hint. Second: I copied the time numbers and forgot the decimal separator (too late in the night :-) ). My older private laptop requires about 6.0-6.5s for the example. I measured again on a bigger machine just now and get about 2s when executing NQueens of size 11 with method 2 or 3. For n=12 and method 2, the big machine requires about 20s with map-stored phantom references, and also 20s with the list-based references. Measuring further examples might not bring any benefit on my side. From theoretical point, the space complexity of list and map are quite similar. General time complexity is also similar. Worst time complexity is better for the list-based approach. -> I will merge this PR. |
I played around with the user propagator feature I implemented in #349 and ran into a performance problem related to phantom references. The issue is that the user propagator creates a
BooleanFormula
wrapper whenever the SMT solver reports a variable assignment, causing a phantom reference to get created.This quickly generates hundreds of thousands of phantom references (or even millions?) and the
HashMap
that is used to manage phantom references degrades heavily: 80%(!) of all solving time was spent just accessing the map (this was 4 minutes from 5 minutes of total solving time!).In this PR I propose a simple (general-purpose) solution by replacing the map with a doubly-linked list.
Doubly-linked lists provide the necessary
O(1)
deletion and insertion time while scaling to arbitrary sizes.Furthermore, the memory footprint of the list should be strictly smaller than that of the previously used
IdentityHashMap
.On my test benchmark, the solving time was reduced from 5 minutes down to 70 seconds, and the phantom reference overhead was just 10% rather than 80% of the total time. Interestingly, my profiler reported the 10% to be mostly from
ReferenceQueue.poll
rather than the actual cleanup code (the profiler might be inaccurate though).