Skip to content
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.

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

gh-102567: Add -X importtime=2 for logging an importtime message for already-loaded modules #118655

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

noahbkim
Copy link
Contributor

@noahbkim noahbkim commented May 6, 2024

As mentioned in the issue:

While -X importtime is incredibly useful for analyzing module import times, by design, it doesn't log anything if an imported module has already been loaded. -X importtime=2 would provide additional output for every module that's already been loaded:

The updated example:

>>> import uuid
import time: cached    | cached     | builtins
import time: cached    | cached     | linecache
import time: cached    | cached     |   _io
import time: cached    | cached     |   os
import time: cached    | cached     |   sys
import time: cached    | cached     |   enum
import time:       594 |        594 |   _uuid
import time:      2428 |       3022 | uuid

Discussion: https://discuss.python.org/t/x-importtrace-to-supplement-x-importtime-for-loaded-modules/23882/5
Prior email chain: https://mail.python.org/archives/list/python-ideas@python.org/thread/GEISYQ5BXWGKT33RWF77EOSOMMMFUBUS/

@Eclips4
Copy link
Member

Eclips4 commented May 6, 2024

Hello!
This PR needs to add:

  1. A NEWS entry
  2. A whatsnew entry
  3. A new paragraph in the Doc/cmdline for the -X option.
  4. New test case in the Lib/test/test_cmd_line

@noahbkim
Copy link
Contributor Author

noahbkim commented May 6, 2024

Hello! This PR needs to add:

  1. A NEWS entry
  2. A whatsnew entry
  3. A new paragraph in the Doc/cmdline for the -X option.
  4. New test case in the Lib/test/test_cmd_line

Thanks for the quick feedback. I've committed a first pass of 1-3. As for tests: there appear to be no importtime tests (likely due to the fact that any simple implementation would be super noisy as other parts of the interpreter change). Would it be sufficient to do some rudimentary check i.e. import foo then ensure a corresponding foo row appears in the output?

Thanks again!

@Eclips4
Copy link
Member

Eclips4 commented May 6, 2024

Hello! This PR needs to add:

  1. A NEWS entry
  2. A whatsnew entry
  3. A new paragraph in the Doc/cmdline for the -X option.
  4. New test case in the Lib/test/test_cmd_line

Thanks for the quick feedback. I've committed a first pass of 1-3. As for tests: there appear to be no importtime tests (likely due to the fact that any simple implementation would be super noisy as other parts of the interpreter change). Would it be sufficient to do some rudimentary check i.e. import foo then ensure a corresponding foo row appears in the output?

Thanks again!

Yes, it would be sufficient :)

@Eclips4
Copy link
Member

Eclips4 commented May 6, 2024

Also, there's a merge conflict. Can you resolve it? (I can do it myself, but it seems like you have turned off this option)

@noahbkim
Copy link
Contributor Author

noahbkim commented May 7, 2024

Also, there's a merge conflict. Can you resolve it? (I can do it myself, but it seems like you have turned off this option)

Done, just added tests as well

@noahbkim
Copy link
Contributor Author

@Eclips4 who should I ping to get final review? Is it alright if I @ the blamed reviewers?

@Eclips4
Copy link
Member

Eclips4 commented May 28, 2024

@Eclips4 who should I ping to get final review? Is it alright if I @ the blamed reviewers?

I guess @vstinner is the right person to review this 😄
However, you should move your changes from whatsnew/3.13.rst to whatsnew/3.14.rst since 3.13 branch is in feature-freeze mode, and this feature will appear only in the 3.14 version.

@Eclips4
Copy link
Member

Eclips4 commented May 28, 2024

However, you should move your changes from whatsnew/3.13.rst to whatsnew/3.14.rst since 3.13 branch is in feature-freeze mode, and this feature will appear only in the 3.14 version.

The same applies for the versionchanged directives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants