Skip to content

Commit

Permalink
perf: cache results of cnf, dnf and _merge_single_markers
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
radoering committed Jun 24, 2023
1 parent 5e85c04 commit 9f58ec7
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 31 deletions.
8 changes: 6 additions & 2 deletions src/poetry/core/constraints/generic/constraint.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@ def intersect(self, other: BaseConstraint) -> BaseConstraint:
return other.intersect(self)

def union(self, other: BaseConstraint) -> BaseConstraint:
if isinstance(other, Constraint):
from poetry.core.constraints.generic.union_constraint import UnionConstraint
from poetry.core.constraints.generic.union_constraint import UnionConstraint

if isinstance(other, Constraint):
if other == self:
return self

Expand All @@ -131,6 +131,10 @@ def union(self, other: BaseConstraint) -> BaseConstraint:

return AnyConstraint()

# to preserve order (functionally not necessary)
if isinstance(other, UnionConstraint):
return UnionConstraint(self).union(other)

return other.union(self)

def is_any(self) -> bool:
Expand Down
2 changes: 1 addition & 1 deletion src/poetry/core/constraints/generic/multi_constraint.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def __eq__(self, other: object) -> bool:
if not isinstance(other, MultiConstraint):
return False

return set(self._constraints) == set(other._constraints)
return self._constraints == other._constraints

def __hash__(self) -> int:
h = hash("multi")
Expand Down
14 changes: 10 additions & 4 deletions src/poetry/core/constraints/generic/union_constraint.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,19 @@ def union(self, other: BaseConstraint) -> BaseConstraint:
our_new_constraints: list[BaseConstraint] = []
their_new_constraints: list[BaseConstraint] = []
merged_new_constraints: list[BaseConstraint] = []
for our_constraint in self._constraints:
for their_constraint in other.constraints:
for their_constraint in other.constraints:
for our_constraint in self._constraints:
union = our_constraint.union(their_constraint)
if union.is_any():
return AnyConstraint()
if isinstance(union, Constraint):
if union not in merged_new_constraints:
if union == our_constraint:
if union not in our_new_constraints:
our_new_constraints.append(union)
elif union == their_constraint:
if union not in their_new_constraints:
their_new_constraints.append(their_constraint)
elif union not in merged_new_constraints:
merged_new_constraints.append(union)
else:
if our_constraint not in our_new_constraints:
Expand Down Expand Up @@ -169,7 +175,7 @@ def __eq__(self, other: object) -> bool:
if not isinstance(other, UnionConstraint):
return False

return set(self._constraints) == set(other._constraints)
return self._constraints == other._constraints

def __hash__(self) -> int:
h = hash("union")
Expand Down
16 changes: 14 additions & 2 deletions src/poetry/core/version/markers.py
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,15 @@ def invert(self) -> BaseMarker:

return parse_marker(f"{self._name} {operator} '{self._value}'")

def __eq__(self, other: object) -> bool:
if not isinstance(other, SingleMarker):
return NotImplemented

return str(self) == str(other)

def __hash__(self) -> int:
return hash(str(self))

def __str__(self) -> str:
return f'{self._name} {self._operator} "{self._value}"'

Expand Down Expand Up @@ -643,7 +652,7 @@ def __eq__(self, other: object) -> bool:
if not isinstance(other, MultiMarker):
return False

return set(self._markers) == set(other.markers)
return self._markers == other.markers

def __hash__(self) -> int:
h = hash("multi")
Expand Down Expand Up @@ -823,7 +832,7 @@ def __eq__(self, other: object) -> bool:
if not isinstance(other, MarkerUnion):
return False

return set(self._markers) == set(other.markers)
return self._markers == other.markers

def __hash__(self) -> int:
h = hash("union")
Expand Down Expand Up @@ -896,6 +905,7 @@ def _compact_markers(
return union(*sub_markers)


@functools.lru_cache(maxsize=None)
def cnf(marker: BaseMarker) -> BaseMarker:
"""Transforms the marker into CNF (conjunctive normal form)."""
if isinstance(marker, MarkerUnion):
Expand All @@ -913,6 +923,7 @@ def cnf(marker: BaseMarker) -> BaseMarker:
return marker


@functools.lru_cache(maxsize=None)
def dnf(marker: BaseMarker) -> BaseMarker:
"""Transforms the marker into DNF (disjunctive normal form)."""
if isinstance(marker, MultiMarker):
Expand Down Expand Up @@ -955,6 +966,7 @@ def union(*markers: BaseMarker) -> BaseMarker:
return min(disjunction, conjunction, unnormalized, key=lambda x: x.complexity)


@functools.lru_cache(maxsize=None)
def _merge_single_markers(
marker1: SingleMarkerLike[SingleMarkerConstraint],
marker2: SingleMarkerLike[SingleMarkerConstraint],
Expand Down
77 changes: 56 additions & 21 deletions tests/constraints/generic/test_constraint.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,10 @@ def test_invert(constraint: BaseConstraint, inverted: BaseConstraint) -> None:
(
Constraint("win32", "!="),
Constraint("linux", "!="),
MultiConstraint(Constraint("win32", "!="), Constraint("linux", "!=")),
(
MultiConstraint(Constraint("win32", "!="), Constraint("linux", "!=")),
MultiConstraint(Constraint("linux", "!="), Constraint("win32", "!=")),
),
),
(
Constraint("win32", "!="),
Expand Down Expand Up @@ -222,21 +225,30 @@ def test_invert(constraint: BaseConstraint, inverted: BaseConstraint) -> None:
(
MultiConstraint(Constraint("win32", "!="), Constraint("linux", "!=")),
MultiConstraint(Constraint("win32", "!="), Constraint("darwin", "!=")),
MultiConstraint(
Constraint("win32", "!="),
Constraint("linux", "!="),
Constraint("darwin", "!="),
(
MultiConstraint(
Constraint("win32", "!="),
Constraint("linux", "!="),
Constraint("darwin", "!="),
),
MultiConstraint(
Constraint("win32", "!="),
Constraint("darwin", "!="),
Constraint("linux", "!="),
),
),
),
],
)
def test_intersect(
constraint1: BaseConstraint,
constraint2: BaseConstraint,
expected: BaseConstraint,
expected: BaseConstraint | tuple[BaseConstraint, BaseConstraint],
) -> None:
assert constraint1.intersect(constraint2) == expected
assert constraint2.intersect(constraint1) == expected
if not isinstance(expected, tuple):
expected = (expected, expected)
assert constraint1.intersect(constraint2) == expected[0]
assert constraint2.intersect(constraint1) == expected[1]


@pytest.mark.parametrize(
Expand Down Expand Up @@ -295,7 +307,10 @@ def test_intersect(
(
Constraint("win32"),
Constraint("linux"),
UnionConstraint(Constraint("win32"), Constraint("linux")),
(
UnionConstraint(Constraint("win32"), Constraint("linux")),
UnionConstraint(Constraint("linux"), Constraint("win32")),
),
),
(
Constraint("win32"),
Expand Down Expand Up @@ -324,8 +339,13 @@ def test_intersect(
(
Constraint("win32"),
UnionConstraint(Constraint("linux"), Constraint("linux2")),
UnionConstraint(
Constraint("win32"), Constraint("linux"), Constraint("linux2")
(
UnionConstraint(
Constraint("win32"), Constraint("linux"), Constraint("linux2")
),
UnionConstraint(
Constraint("linux"), Constraint("linux2"), Constraint("win32")
),
),
),
(
Expand Down Expand Up @@ -366,8 +386,13 @@ def test_intersect(
(
UnionConstraint(Constraint("win32"), Constraint("linux")),
UnionConstraint(Constraint("win32"), Constraint("darwin")),
UnionConstraint(
Constraint("win32"), Constraint("linux"), Constraint("darwin")
(
UnionConstraint(
Constraint("win32"), Constraint("linux"), Constraint("darwin")
),
UnionConstraint(
Constraint("win32"), Constraint("darwin"), Constraint("linux")
),
),
),
(
Expand All @@ -377,11 +402,19 @@ def test_intersect(
UnionConstraint(
Constraint("win32"), Constraint("cygwin"), Constraint("darwin")
),
UnionConstraint(
Constraint("win32"),
Constraint("linux"),
Constraint("darwin"),
Constraint("cygwin"),
(
UnionConstraint(
Constraint("win32"),
Constraint("linux"),
Constraint("darwin"),
Constraint("cygwin"),
),
UnionConstraint(
Constraint("win32"),
Constraint("cygwin"),
Constraint("darwin"),
Constraint("linux"),
),
),
),
(
Expand Down Expand Up @@ -412,10 +445,12 @@ def test_intersect(
def test_union(
constraint1: BaseConstraint,
constraint2: BaseConstraint,
expected: BaseConstraint,
expected: BaseConstraint | tuple[BaseConstraint, BaseConstraint],
) -> None:
assert constraint1.union(constraint2) == expected
assert constraint2.union(constraint1) == expected
if not isinstance(expected, tuple):
expected = (expected, expected)
assert constraint1.union(constraint2) == expected[0]
assert constraint2.union(constraint1) == expected[1]


def test_difference() -> None:
Expand Down
2 changes: 1 addition & 1 deletion tests/version/test_markers.py
Original file line number Diff line number Diff line change
Expand Up @@ -1312,8 +1312,8 @@ def test_union_should_drop_markers_if_their_complement_is_present(
),
MarkerUnion(
SingleMarker("python_version", "<3.7"),
SingleMarker("sys_platform", "!=linux"),
SingleMarker("python_version", ">=3.9"),
SingleMarker("sys_platform", "!=linux"),
),
),
),
Expand Down

0 comments on commit 9f58ec7

Please sign in to comment.