perf: cache results of cnf, dnf and _merge_single_markers

[radoering]

Picking up the suggestion in python-poetry/poetry#7257 (comment)

In order to cache cnf and dnf, we can no longer consider composite markers and constraints with the same sub elements but a different order to be equal because order is relevant when building an intersection or union. Neglecting order leads to different results if the result of an "equal" marker with a different order has already been cached. This can even result in a RecursionError, as the test suite demonstrates.

Performance measurements see python-poetry/poetry#7257 (comment)

[dimbleby]

In order to cache cnf and dnf, we can no longer consider composite markers and constraints with the same sub elements but a different order to be equal because order is relevant when building an intersection or union. Neglecting order leads to different results if the result of an "equal" marker with a different order has already been cached. This can even result in a RecursionError, as the test suite demonstrates.

this all makes me a bit uncomfortable:

• in my mental model markers with the same set of sub-markers really should be equal
• it's worrying that the code is so close to falling into recursion errors etc, perhaps it's all more fragile than we thought

on the other hand there are quite a lot of tests for this stuff, and if they're all passing then I suppose it's probably fine...!

should __hash__ implementations be updated to match updated __eq__? instead of xor-ing in each individual marker (which was order-independent) it might make more sense to xor in hash(tuple(self._markers))? Or just construct a tuple that goes eg ("union", *self._markers) and hash that.

(If they're ordered, perhaps submarkers should be a tuple rather than a list all along, so as to make them immutable??)

re the SingleMarker: converting to a string for hash / equality purposes feels a bit weird? A typical trick is to define a method that returns a tuple of all the identifying features and use that for both hash and equality. I suppose that's kinda what you've done but with a string instead of a tuple...

[radoering]

• in my mental model markers with the same set of sub-markers really should be equal

I don't think so (anymore). It's just a matter of concept what's defined as equal.

If markers with the same subset of markers should be equal, shouldn't (more generally) markers that can be transformed into each other be equal, too? However, I don't think that works well with our algorithm.

IMO, a better concept is that only markers that have the same string representation are equal. It's easier to achieve and fits better with our algorithm.

should __hash__ implementations be updated to match updated __eq__?

It's not necessary but it might make sense if the hash calculation does not become more expensive so we get less different markers with equal hashes and thus have to do less comparisons for equality.

(If they're ordered, perhaps submarkers should be a tuple rather than a list all along, so as to make them immutable??)

IIRC, we have always considered submarkers immutable so making them a tuple instead of a list might make sense.

re the SingleMarker: converting to a string for hash / equality purposes feels a bit weird? A typical trick is to define a method that returns a tuple of all the identifying features and use that for both hash and equality. I suppose that's kinda what you've done but with a string instead of a tuple...

Agreed.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication