Skip to content

Conversation

@hauntsaninja
Copy link
Contributor

Note this will change if we add or remove modules from the standard library. We could alternatively hardcode a list of standard library modules.

Note this will change if we add or remove modules from the standard library. We could alternatively hardcode a list of standard library modules.
Copy link
Contributor

@mdboommdboom left a comment

Choose a reason for hiding this comment

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

This is a good idea in general -- the standard library is likely to cover a lot of ground, and is definitely "real code people actually use".

However, this benchmark is likely to change even when the contents of the stdlib changes (not just when files are added/removed). So I think the only way to make a stable benchmark out of this would be to actually include a copy of the stdlib here. That's probably reasonably forward-compatible to new versions of Python (at least as much as any random library).

Given that would be kind of a weird thing to do, I guess I'd like to get some other feedback to see what others think before merging this.

@hauntsaninja
Copy link
ContributorAuthor

As a user, I definitely care about stdlib import time, so in my mind that was maybe more of a feature.
But maybe instead we should make this test import a few version-pinned popular third party dependencies; this will reduce noise from incidental changes and weight in favour of stdlib modules that are widely used.

@mdboom
Copy link
Contributor

You make some good points. I think there's a couple of different frames of reference here. If the goal is "how fast is CPython at importing all this code?", copying the code makes sense. If the goal is "how fast does the stdlib take to import for CPython version X.Y", this PR as-is makes a lot of sense. I suppose as long as we document what it's good for to avoid confusion, that's fine. (We don't really have a good place to document those kind of things yet, but a comment in the run_benchmark.py would be good enough for now).

Copy link
Contributor

@mdboommdboom left a comment

Choose a reason for hiding this comment

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

If the comment I suggested makes sense to you, I think this is good to merge. Feel free to edit it if it's not clear, etc.

@@ -0,0 +1,29 @@
import os
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
importos
"""
Measuresthetimeittakestoimportallofthemodulesinthestdlib.
Thisbenchmarkisexpectedtochangeasthestdlibchanges, soisnot
suitableformeasuringcompilationtime.
"""
importos

@hugovk
Copy link
Member

Let's merge this.

@hauntsaninja Include the suggested change if you like, otherwise let's just merge.

Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

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

Agree with Hugo.

One proposed change that might reduce benchmark variance, as stdlib_module_names is an unsorted set:

Comment on lines +16 to +27
with open(main, "w") as f:
f.write("""
import importlib
import sys
for m in sys.stdlib_module_names:
if m in{"antigravity", "this"}:
continue
try:
importlib.import_module(m)
except ImportError:
pass
""")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
withopen(main, "w") asf:
f.write("""
importimportlib
importsys
forminsys.stdlib_module_names:
ifmin{"antigravity", "this"}:
continue
try:
importlib.import_module(m)
exceptImportError:
pass
""")
modules_to_import=sorted(sys.stdlib_module_names-{'antigravity', 'this'})
withopen(main, 'w', encoding='utf-8') asf:
formoduleinmodules_to_import:
f.write(f"""
try:
import{module}
exceptImportError:
pass
""")

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@hauntsaninja@mdboom@hugovk@AA-Turner