performance fix for simplified marker simplifications

[radoering]

While working on full support for overlapping markers, I encountered a combinatorial explosion when building a union of markers (see test case).

The issue is that we introduced cnf without introducing a counterpart of union_simplify, which leads to unnecessary big markers. Therefore, I added intersect_simplify analoguous to union_simplify.

Further, the simplifications in #530 went a bit too far in removing the common_markers/unique_markers simplification in union_simplify. I reverted this removal and added analogue functionality to intersect_simplify.

Comparing the number of items in itertools.product in dnf of union (after cnf) shows the benefit of the simplifications:

union (see test case)	number and length of markers in conjunction after cnf	number of items in itertools.product in dnf
without PR	31 (1-4)	> 2**44
with simple intersect_simplifiy	9 (1-2)	256
with revival of common_markers/unique_markers simplification	3 (1-2)	4

[dimbleby]

There's a possible missed opportunity here - though I suspect not an important one.

unique_intersection could be EmptyMarker - actually that feels relatively likely and, having got this far, we ought to take the simplification in that case.

ie suggest if isinstance(unique_intersection, (SingleMarker, EmptyMarker)):

You could restore symmetry by testing if isinstance(unique_union, (SingleMarker, AnyMarker)): in the other place

[radoering]

You could restore symmetry by testing if isinstance(unique_union, (SingleMarker, AnyMarker)): in the other place

In the other place we can do better by testing if not isinstance(unique_union, MarkerUnion):. Doing the same with MultiMarker in this place is not possible because we always prefer dnf:

poetry-core/src/poetry/core/version/markers.py

Lines 775 to 784 in 5a273c3

	def intersection(*markers: BaseMarker) -> BaseMarker:
	return dnf(MultiMarker(*markers))
	def union(*markers: BaseMarker) -> BaseMarker:
	conjunction = cnf(MarkerUnion(*markers))
	if not isinstance(conjunction, MultiMarker):
	return conjunction
	return dnf(conjunction)

[dimbleby]

In the other place we can do better by testing if not isinstance(unique_union, MarkerUnion)

I don't think this is doing better, though, because it's never possible to achieve the other types. ie I think that the two different formulations are equivalent, one of them just restores symmetry.

[dimbleby]

By the way I am not absolutely 100% mathematically certain of the claim that unique_union can't be a MultiMarker - but it is at any rate difficult enough to make it happen that I can't figure out how to do it (and also rare enough that the unit tests don't achieve it either).

Comparing the value of the possible micro-optimisation in case a MultiMarker does occur here against the value of slightly prettier code - I'd find it hard to make a strong case either way. Personally I am drawn to the symmetry but no big deal.

[radoering]

unique_union should be a MultiMarker for something like

unique_markers = {'python_version >= "3.7"', 'python_version < "3.11"'}
other_unique_markers = {'python_version >= "3.8"', 'python_version < "3.12"'}

However, due to the nature of dnf, I believe there will always be another marker with only one of the unique markers so the markers will be merged later anyway. As you said it's probably just a micro-optimization.

I just realized that allowing MultiMarker would require to use lists instead of sets for unique_markers and other_unique_markers to keep deterministic results. Maybe, that's what tips the scales. So let's choose symmetry for now until there is a good reason to choose the opposite.

[dimbleby]

This saddens me a little, I was quite pleased with the removal of all the special cases! But the testcase is rather convincing, all the more so if it was encountered in the real world.

Left a couple of more or less pedantic comments

[radoering]

Great suggestions. 👍

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
1.0% Duplication