From 281c600a6681f32dac1dc567642adc384569a493 Mon Sep 17 00:00:00 2001 From: Thomas Bittar Date: Tue, 24 Mar 2026 22:41:52 +0100 Subject: [PATCH] =?UTF-8?q?perf:=20fix=20O(T=C2=B2)=20list=20copies=20in?= =?UTF-8?q?=20CopyVisitor.addition?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default `addition` in ExpressionVisitorOperations accumulated results with `res = res + o` in a loop. The flattening `__add__` on ExpressionNode copies the accumulated operand list at each step, giving O(T²) total copy work for a T-term AdditionNode. Override `addition` in CopyVisitor to build the AdditionNode directly from a list comprehension, reducing the cost to O(T). Add a regression test that asserts the time ratio between T=10_000 and T=1_000 stays below 20 (linear ≈ 10, quadratic ≈ 100). Co-Authored-By: Claude Sonnet 4.6 --- src/gems/expression/copy.py | 4 ++ .../expressions/visitor/test_copy.py | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/src/gems/expression/copy.py b/src/gems/expression/copy.py index a5276ccd..37eae951 100644 --- a/src/gems/expression/copy.py +++ b/src/gems/expression/copy.py @@ -14,6 +14,7 @@ from typing import List, cast from .expression import ( + AdditionNode, AllTimeSumNode, CeilNode, ComparisonNode, @@ -44,6 +45,9 @@ class CopyVisitor(ExpressionVisitorOperations[ExpressionNode]): Simply copies the whole AST. """ + def addition(self, node: AdditionNode) -> ExpressionNode: + return AdditionNode([visit(o, self) for o in node.operands]) + def literal(self, node: LiteralNode) -> ExpressionNode: return LiteralNode(node.value) diff --git a/tests/unittests/expressions/visitor/test_copy.py b/tests/unittests/expressions/visitor/test_copy.py index 52a20b3f..3727a759 100644 --- a/tests/unittests/expressions/visitor/test_copy.py +++ b/tests/unittests/expressions/visitor/test_copy.py @@ -11,6 +11,8 @@ # This file is part of the Antares project. +import time + from gems.expression import ( AdditionNode, DivisionNode, @@ -29,6 +31,44 @@ ) +def test_copy_large_addition_is_linear() -> None: + """ + Copying an AdditionNode with T operands must be O(T), not O(T²). + + Before the fix, CopyVisitor inherited the default addition() from + ExpressionVisitorOperations which accumulated results with `res = res + o`. + Each call to __add__ flattens the AdditionNode by copying the accumulated + operand list, giving 1+2+...+(T-1) = O(T²) list copies in total. + + The fix overrides addition() in CopyVisitor to build AdditionNode directly + from a list comprehension, reducing the cost to O(T). + + We verify linearity by checking that the time ratio between T=10_000 and + T=1_000 stays below 20 (linear ≈ 10, quadratic ≈ 100). + """ + small_n = 1_000 + large_n = 10_000 + + small_node = AdditionNode([VariableNode(f"x{i}") for i in range(small_n)]) + large_node = AdditionNode([VariableNode(f"x{i}") for i in range(large_n)]) + + t0 = time.perf_counter() + copy_expression(small_node) + small_time = time.perf_counter() - t0 + + t0 = time.perf_counter() + copy_expression(large_node) + large_time = time.perf_counter() - t0 + + ratio = large_time / small_time + assert ratio < 20, ( + f"copy_expression scaling looks super-linear: " + f"T={small_n} took {small_time:.4f}s, " + f"T={large_n} took {large_time:.4f}s, " + f"ratio={ratio:.1f} (expected <20 for O(T))" + ) + + def test_copy_ast() -> None: ast = AllTimeSumNode( DivisionNode(