Skip to content

gh-142145: Avoid timing measurements in quadratic behavior test #143105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
colesbury merged 3 commits into from
Dec 24, 2025
Merged

gh-142145: Avoid timing measurements in quadratic behavior test #143105

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc9cf09
gh-142145: Avoid timing measurements in quadratic behavior test
colesbury Dec 23, 2025
506e978
Lint
colesbury Dec 23, 2025
ec362b3
Changes from review
colesbury Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 32 additions & 19 deletions Lib/test/test_minidom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import copy
import pickle
import time
import io
from test import support
import unittest
Expand Down Expand Up @@ -174,27 +173,41 @@ def testAppendChild(self):
self.assertEqual(dom.documentElement.childNodes[-1].data, "Hello")
dom.unlink()

@support.requires_resource('cpu')
Copy link
Copy Markdown
Member

@gpshead gpshead Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this? i realize the test shouldn't be "slow" now, but the intent of adding it was to mark it as a test that may not be suitable for slow builds and loaded systems. some of our buildbots run with the cpu resource disabled for that reason. it's more of a performance test no matter how it is implemented.

for tests where it isn't platform specific and we just need something in our support tiers to catch an unlikely regression the issue, resource tags save effort.

Copy link
Copy Markdown
Contributor Author

@colesbury colesbury Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add it back. I removed it because my understanding was that "cpu" was for tests that were CPU-heavy, and now this test is very fast (~30 ms in a debug build).

def testAppendChildNoQuadraticComplexity(self):
# Don't use wall-clock timing (too flaky). Instead count a proxy for the
Copy link
Copy Markdown
Member

@gpshead gpshead Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elide the model's explanatory comment about the old no longer present code.

# old quadratic behavior: repeated attribute access, such as of
# parentNode/nodeType during document-membership checks.
impl = getDOMImplementation()

newdoc = impl.createDocument(None, "some_tag", None)
top_element = newdoc.documentElement
children = [newdoc.createElement(f"child-{i}") for i in range(1, 2 ** 15 + 1)]
element = top_element

start = time.monotonic()
for child in children:
element.appendChild(child)
element = child
end = time.monotonic()

# This example used to take at least 30 seconds.
# Conservative assertion due to the wide variety of systems and
# build configs timing based tests wind up run under.
# A --with-address-sanitizer --with-pydebug build on a rpi5 still
# completes this loop in <0.5 seconds.
self.assertLess(end - start, 4)
def work(n):
doc = impl.createDocument(None, "some_tag", None)
element = doc.documentElement
total_calls = 0

def getattribute_counter(self, attr):
nonlocal total_calls
total_calls += 1
return object.__getattribute__(self, attr)

with support.swap_attr(Element, "__getattribute__", getattribute_counter):
Copy link
Copy Markdown
Member

@gpshead gpshead Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing this is... gross, but likely works. your assertGreater(w1, 0) below covers the case when it stops doing so. LGTM

the other thought i was having as i did the simple "raise the timeout to 4s" to unblock the earlier ones was that we don't need absolute times at all. measuring relative time taken as we increase the amount of work to ensure that it scales - similar to this scaling measurement of attribute accesses - would be sufficient and should produce similar results on super slow builds or heavily loaded systems. it would still be time based (so maybe use resource.getrusage(resource.RUSAGE_SELF).ru_utime instead of time. APIs... though i suspect rusage is low-res) but far less "works on my system"

we have a smattering of other timing based cpu performancy tests in the repo. i don't think we've codified a reusable best practice.

Copy link
Copy Markdown
Contributor Author

@colesbury colesbury Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

measuring relative time taken as we increase the amount of work ... would be sufficient

It'd be great to have some sort of standard best practice for this. If resource.getrusage(resource.RUSAGE_SELF).ru_utime isn't too affected by other processes, that would be good.

Some other things I was thinking about:

  1. Count cycles (via perf_event_open?)
  2. Count bytecodes executed (by instrumenting ceval.c in debug builds)

for _ in range(n):
child = doc.createElement("child")
element.appendChild(child)
element = child
return total_calls

# Doubling N should not ~quadruple the work.
w1 = work(1024)
w2 = work(2048)
w3 = work(4096)

self.assertGreater(w1, 0)
r1 = w2 / w1
r2 = w3 / w2
self.assertLess(
max(r1, r2), 3.2,
msg=f"Possible quadratic behavior: work={w1,w2,w3} ratios={r1,r2}"
)

def testSetAttributeNodeWithoutOwnerDocument(self):
# regression test for gh-142754
Expand Down
Loading