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

Do not ask for equals/hashCode on the sequential specification until necessary #135

Closed
ndkoval opened this issue Nov 25, 2022 · 12 comments
Closed
Assignees

Comments

@ndkoval
Copy link
Collaborator

ndkoval commented Nov 25, 2022

Usually, the verification phase is super-fast, even with default equals and hashCode implementations. However, it might be critical to provide custom implementations when testing scenarios are large or when the data structure operations are expensive.

Let's not require equals/hashCode to be implemented if the verification phase takes excepted time -- Lincheck should neither throw an exception nor print a warning. However, when the verification process takes an unexpectedly long time, it would be helpful to print a warning with instructions on how to implement equals and hashCode.

@ndkoval
Copy link
Collaborator Author

ndkoval commented Mar 15, 2023

These messages are really annoying...

To make verification faster, you can specify the state equivalence relation on your sequential specification.
At the current moment, `MultiMapTest` does not specify it, or the equivalence relation implementation is incorrect.
To fix this, please implement `equals()` and `hashCode()` functions on `MultiMapTest`; the simplest way is to extend `VerifierState`
and override the `extractState()` function, which is called at once and the result of which is used for further `equals()` and `hashCode()` invocations.

@ndkoval
Copy link
Collaborator Author

ndkoval commented Mar 16, 2023

We need to make experiments to decide when equals/hashCode implementations start helping. After that, we can have heuristics that enable asking for equals/hashCode only when it seems necessary.

@ndkoval
Copy link
Collaborator Author

ndkoval commented Mar 29, 2023

Let's also remove the requireStateEquivalenceImplCheck parameter: I cannot find a single usage.

@avpotapov00
Copy link
Collaborator

avpotapov00 commented Apr 6, 2023

As experiments have shown, there is no explicit correlation between equals/hashcode presence and verification time and a whole test time. Equals and hashcode can significantly improve or mess up performance only if a scenario is really big. In real tasks it can be if threads count is >= 4.
We measured the perfomace with and without equals/hashcode on the tests below:

  • BufferedChannel
  • ConcurrentHashMap
  • ConcurrentLinkedDeque
  • Counter
  • ConcurrentSkipListMap
  • LockFreeTaskQueue (QuiescentConsistency)
  • ConcurrentHashMap

The graph below describes whether equals / hashcode improves or worsens verification time. The x-axis value is equal to the difference between the verification time without equals / hashcode and with equals / hashcode in milliseconds for the same experiment. The y-axis is the number of threads.
verification_time_delta__threads

In most tests, there is not much difference, but on those where the difference is significant, the presence of equals / hashcode often worsens the results.
The lack of correlation is explained as follows: although the ability to reuse states in an LTS graph reduces the graph, making its traversal faster, sometimes the comparison methods, if they are present, are not fast enough, so verification takes longer.

According to the current results of the experiments, it was decided to mark VeriferState as @deprecated and completely remove the warning about the absence of equals / hashcode, since this option can not only improve, but also worsen the running time and also complicates the use of the framework.

@ndkoval
Copy link
Collaborator Author

ndkoval commented Apr 6, 2023

These results are really surprising, thanks @avpotapov00 for the analysis!

@ndkoval
Copy link
Collaborator Author

ndkoval commented Apr 6, 2023

@avpotapov00, could you please check whether it is better to use IdentityHashMap in the LTS structure then?

@avpotapov00
Copy link
Collaborator

avpotapov00 commented Apr 6, 2023

Experiments have shown the following results:
histogram

@ndkoval
Copy link
Collaborator Author

ndkoval commented Apr 6, 2023

It would be better to show the percentage of the time spent in the verification phase.

@avpotapov00
Copy link
Collaborator

Here it is.
percent

@avpotapov00
Copy link
Collaborator

So I propose to keep current HashMap states holder implementation in LTS structure, to deprecate VerifierState and remove the requireStateEquivalenceImplCheck parameter. Annoying warnings about equals/hashcode will be removed as well.

@eupp
Copy link
Collaborator

eupp commented Apr 10, 2023

@avpotapov00
then we can also remove checkStateEquivalenceImplementation from the Verifier interface, and make it SAM interface.

@ndkoval
Copy link
Collaborator Author

ndkoval commented Apr 18, 2023

fixed in #160

@ndkoval ndkoval closed this as completed Apr 18, 2023
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

When branches are created from issues, their pull requests are automatically linked.

3 participants