From 03d08d74cb744a68adc16d09287f28ff3f0c6a3c Mon Sep 17 00:00:00 2001 From: Stanislav Date: Fri, 16 Oct 2026 10:04:31 +0400 Subject: [PATCH] insert_rule: find the rule to replace through an index, not by a full scan When a new unconditional rule is added, insert_rule looked for a rule with the same pattern by calling sameQ on every rule already stored, so building a definition of n rules took O(n^2) time. A package of ~90000 assignments did not load in 15 minutes; 1000 assignments to a symbol that already has 4000 DownValues took 19 s. Each Definition now keeps, for each rule list, an index of its unconditional rules by a fingerprint of their patterns: the structure of the expression with integers, strings and symbols, all other atoms lumped together, so that SameQ patterns always share a fingerprint. sameQ is then called only for the rules in that bucket. The index belongs to the list object it was built for and is rebuilt when the list is replaced (ClearAll, DownValues[f] = ...); rules removed by Unset are dropped from it when met. The precedence key cannot serve as this index: the same left-hand side gets a different key when its head, or an inner head, gets attributes like Flat between two assignments. Behaviour is unchanged; the 1000 assignments now take 0.45 s. Fixes #1994. Co-Authored-By: Claude Opus 5.5 --- mathics/core/definitions.py | 132 +++++++++++++++++++++++++++++----- test/core/test_definitions.py | 96 +++++++++++++++++++++++++ 2 files changed, 209 insertions(+), 19 deletions(-) diff --git a/mathics/core/definitions.py b/mathics/core/definitions.py index c6f3dc8132..3006ac9149 100644 --- a/mathics/core/definitions.py +++ b/mathics/core/definitions.py @@ -10,7 +10,7 @@ import pickle import re from collections import defaultdict -from typing import Dict, List, Optional, Sequence, Set, Tuple, Union +from typing import Dict, Hashable, List, Optional, Sequence, Set, Tuple, Union from mathics_scanner.tokeniser import full_names_pattern @@ -79,6 +79,12 @@ def __init__( self.options: dict[str, BaseElement] = rules_dict.get("options", {}) self.messages = rules_dict.get("messages", []) + # For each rule list, an index of its unconditional rules by the + # fingerprint of their patterns, see ``insert_rule()``. It belongs to + # the list object it was built for and is rebuilt when the list is + # replaced. + self.rule_index: Dict[str, Tuple[list, dict]] = {} + self.is_numeric = is_numeric self.attributes = attributes self.builtin = builtin @@ -117,9 +123,29 @@ def add_rule_at(self, rule: BaseRule, rule_list_name: str) -> bool: Add `rule` to the set of rules in field with attribute `rule_list_name` """ values = self.get_values_list(rule_list_name) - insert_rule(values, rule) + insert_rule(values, rule, self.get_rule_index(rule_list_name, values)) return True + def get_rule_index(self, rule_list_name: str, values: List[BaseRule]) -> dict: + """ + Return the index of the unconditional rules in `values`, the rule + list `rule_list_name`, by pattern fingerprint. + """ + if not hasattr(self, "rule_index"): + # A Definition pickled before rule_index existed. + self.rule_index = {} + indexed = self.rule_index.get(rule_list_name) + if indexed is not None and indexed[0] is values: + return indexed[1] + index: Dict[Hashable, List[BaseRule]] = {} + for existing in values: + if not is_conditional_rule(existing): + index.setdefault(pattern_fingerprint(existing.pattern.expr), []).append( + existing + ) + self.rule_index[rule_list_name] = (values, index) + return index + def get_values_list(self, rule_list_name: str) -> list[BaseRule]: """Return a Rule value lists""" assert hasattr(self, rule_list_name) @@ -975,7 +1001,38 @@ def determine_value_role( return None -def insert_rule(values: List[BaseRule], rule: BaseRule) -> None: +def is_conditional_rule(rule: BaseRule) -> bool: + """ + Check if the replacement rule is a conditional replacement. + FunctionApplyRules are always considered "conditional", while + replacement rules are conditional if the replace attribute is + a conditional expression. + """ + return not hasattr(rule, "replace") or rule.replace.has_form(SymbolCondition, 2) + + +def pattern_fingerprint(expr: BaseElement) -> Hashable: + """ + Return a hashable summary of `expr` such that expressions that are + SameQ have the same fingerprint. Integers, strings and symbols are + told apart; all other atoms, whose SameQ may be looser (e.g. Reals + of different precision), share one fingerprint. + """ + if isinstance(expr, Symbol): + return expr.name + if isinstance(expr, (Integer, String)): + return (type(expr).__name__, expr.value) + if isinstance(expr, Atom): + return None + return ( + pattern_fingerprint(expr.get_head()), + tuple(pattern_fingerprint(element) for element in expr.get_elements()), + ) + + +def insert_rule( + values: List[BaseRule], rule: BaseRule, index: Optional[dict] = None +) -> None: """ Add a new rule inside a list of values. Rules are sorted in a way that the first elements @@ -990,34 +1047,71 @@ def insert_rule(values: List[BaseRule], rule: BaseRule) -> None: A list of rules. rule : Rule A new rule. + index : dict, optional + The unconditional rules of `values` by pattern fingerprint, as + built by ``Definition.get_rule_index()``; kept up to date here. + With it, `rule` is compared only with the rules whose patterns + have its fingerprint. Without it, with every rule in `values`, + which makes building a definition of n rules take O(n^2) time. """ - def is_conditional(x) -> bool: - """ - Check if the replacement rule is a conditional replacement. - FunctionApplyRules are always considered "conditional", while - replacement rules are conditional if the replace attribute is - a conditional expression. - """ - return not hasattr(x, "replace") or x.replace.has_form(SymbolCondition, 2) - # If the rule is not conditional, and there are # equivalent rules which are not conditional either, # remove them. - if not is_conditional(rule): - for index, existing in enumerate(values): - if is_conditional(existing): - continue - if existing.pattern.sameQ(rule.pattern): - del values[index] - break + if not is_conditional_rule(rule): + if index is None: + for position, existing in enumerate(values): + if is_conditional_rule(existing): + continue + if existing.pattern.sameQ(rule.pattern): + del values[position] + break + else: + bucket = index.setdefault(pattern_fingerprint(rule.pattern.expr), []) + for existing in list(bucket): + if existing.pattern.sameQ(rule.pattern): + bucket.remove(existing) + found = find_rule(values, existing) + if found is not None: + del values[found] + break + # `existing` was removed from `values` some other way + # (e.g. by Unset); look further. + bucket.append(rule) # use insort_left to guarantee that if equal rules exist, newer rules will # get higher precedence by being inserted before them. see DownValues[]. bisect.insort_left(values, rule, key=lambda x: x.pattern_precedence) +def find_rule(values: List[BaseRule], rule: BaseRule) -> Optional[int]: + """ + Return the position of the very object `rule` in `values`, or None. + `values` is normally sorted by ``pattern_precedence`` (``insert_rule()`` + keeps it so), and then only the rules with the precedence of `rule` + are looked at. + """ + precedence = rule.pattern_precedence + try: + start = bisect.bisect_left( + values, precedence, key=lambda x: x.pattern_precedence + ) + end = bisect.bisect_right( + values, precedence, lo=start, key=lambda x: x.pattern_precedence + ) + except TypeError: + start, end = 0, 0 + for position in range(start, end): + if values[position] is rule: + return position + # Not sorted, e.g. after DownValues[f] = {...}: look everywhere. + for position, existing in enumerate(values): + if existing is rule: + return position + return None + + def merge_definitions(candidates: List[Definition]) -> Definition: """Build a Definition object by merging other definitions""" rules: dict = { diff --git a/test/core/test_definitions.py b/test/core/test_definitions.py index 2a4bdc3327..6e6c56eb37 100644 --- a/test/core/test_definitions.py +++ b/test/core/test_definitions.py @@ -3,11 +3,16 @@ Tests functions in mathics.core.definition """ +from test.helper import check_evaluation, session + import pytest +from mathics.core.atoms import Integer from mathics.core.definitions import determine_value_role +from mathics.core.expression import Expression from mathics.core.parser import parse_builtin_rule from mathics.core.pattern import BasePattern +from mathics.core.symbols import Symbol def load_pattern_objects(): @@ -124,3 +129,94 @@ def test_determine_value_role(pattern_str, tag, position): pattern_pat = BasePattern.create(pattern_expr) assert determine_value_role(pattern_pat, target) == position assert determine_value_role(pattern_expr, target) == position + + +@pytest.mark.parametrize( + ("str_expr", "str_expected", "assert_failure_msg"), + [ + ( + "ClearAll[insF]; insF[1] = a; insF[2] = b; insF[1] = c; DownValues[insF]", + "{HoldPattern[insF[1]] :> c, HoldPattern[insF[2]] :> b}", + "A new rule replaces the rule with the same pattern", + ), + ( + "ClearAll[insF]; insF[x_] := 1 /; x > 0; insF[x_] := 2; insF[x_] := 3 /; x > 0; " + "Length[DownValues[insF]]", + "3", + "Conditional rules are never replaced", + ), + ( + "ClearAll[insF]; insF[1] = a; SetAttributes[insF, Flat]; insF[1] = b; " + "DownValues[insF]", + "{HoldPattern[insF[1]] :> b}", + "Also when the head got attributes between the two assignments", + ), + ( + "ClearAll[insF, insH]; insF[insH[1]] = a; SetAttributes[insH, OneIdentity]; " + "insF[insH[1]] = b; DownValues[insF]", + "{HoldPattern[insF[insH[1]]] :> b}", + "Also when an inner head got attributes between the two assignments", + ), + ( + "ClearAll[insF]; insF[1] = a; insF[2] = b; insF[1] =.; insF[1] = c; insF[1] = d; " + "DownValues[insF]", + "{HoldPattern[insF[1]] :> d, HoldPattern[insF[2]] :> b}", + "Also after Unset", + ), + ("ClearAll[insF, insH];", "Null", None), + ], +) +def test_insert_rule_replaces_same_pattern(str_expr, str_expected, assert_failure_msg): + check_evaluation( + str_expr, + str_expected, + failure_message=assert_failure_msg, + ) + + +def test_insert_rule_after_explicit_order(): + """ + After ``DownValues[f] = {...}`` the rules keep the order given, which + need not be the precedence order. A new rule must still replace the + rule with the same pattern. + """ + from mathics.core.definitions import Definition + from mathics.core.rules import RewriteRule + + f = Symbol("Global`insK") + lhs = [Expression(f, Integer(k)) for k in range(1, 6)] + definition = Definition(name=f.name) + # The reverse of the precedence order. + definition.set_values_list( + "downvalues", [RewriteRule(expr, Symbol("old")) for expr in reversed(lhs)] + ) + definition.add_rule(RewriteRule(lhs[0], Symbol("new"))) + assert len(definition.downvalues) == 5 + assert [ + r.replace for r in definition.downvalues if r.pattern.expr.sameQ(lhs[0]) + ] == [Symbol("new")] + + +def test_insert_rule_compares_few_rules(monkeypatch): + """ + Adding a rule should not compare its pattern with every rule already + stored: with n rules that makes loading a definition O(n^2). + """ + session.evaluate("ClearAll[insG]; Do[insG[i, 2] = i, {i, 1, 500}]") + calls = 0 + same_q = BasePattern.sameQ + + def counting_same_q(self, other): + nonlocal calls + calls += 1 + return same_q(self, other) + + monkeypatch.setattr(BasePattern, "sameQ", counting_same_q) + session.evaluate("insG[501, 2] = 501; insG[250, 2] = -250") + monkeypatch.undo() + assert session.evaluate("{Length[DownValues[insG]], insG[250, 2]}").to_python() == ( + 501, + -250, + ) + session.evaluate("ClearAll[insG]") + assert calls < 10, f"{calls} pattern comparisons for two assignments"