-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
bpo-31415: Add -X importtime
option.
#3490
Changes from 1 commit
280d585
cd1f9a6
bb03fc3
08cd26f
62019f7
c915ef0
853c0f6
7e48d5d
4a2f121
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Add ``-X importprofile`` option to show how long each import takes. It can | ||
be used to optimize application's startup time. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1666,10 +1666,42 @@ PyImport_ImportModuleLevelObject(PyObject *name, PyObject *globals, | |
} | ||
} | ||
else { | ||
static int import_level; | ||
_Py_IDENTIFIER(importprofile); | ||
int import_profile = 0; | ||
_PyTime_t t1=0, t2; | ||
|
||
Py_XDECREF(mod); | ||
|
||
/* XOptions is initialized after first some imports. | ||
* So we can't have negative cache. | ||
* Anyway, importlib.__find_and_load is much slower than | ||
* _PyDict_GetItemId() | ||
*/ | ||
PyObject *xoptions = PySys_GetXOptions(); | ||
if (xoptions) { | ||
PyObject *value = _PyDict_GetItemId(xoptions, &PyId_importprofile); | ||
import_profile = (value == Py_True); | ||
} | ||
|
||
if (import_profile) { | ||
import_level++; | ||
t1 = _PyTime_GetMonotonicClock(); | ||
} | ||
|
||
mod = _PyObject_CallMethodIdObjArgs(interp->importlib, | ||
&PyId__find_and_load, abs_name, | ||
interp->import_func, NULL); | ||
|
||
if (import_profile) { | ||
import_level--; | ||
t2 = _PyTime_GetMonotonicClock(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. _PyTime_t has no unit. You cannot use directly t2-t1. I suggest you to use _PyTime_AsMicroseconds(t2 - t1, _PyTime_ROUND_CEILING). Moreover, it will round "correctly" ;-) (towards +inf, rather than towards zero). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
fprintf(stderr, "%*s- %s %ld [us]\n", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In your sample, the output looks nice. But imports can emit warnings, importlib can log extrat messages (try python3 -v), etc. It would help parsing to prefix messages. For example: "import_time: (...)." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated format, and output example in pull request comment. |
||
import_level*2, "", | ||
PyUnicode_AsUTF8(abs_name), | ||
(long)(t2 - t1) / 1000); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to use the ms unit if us >= 10 000? It would make the output easier to read, no? I don't understand the "123 [us]" format. Why not "123 us"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would vote for "123 us" too, or "[123 us]". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Square brackets is used to make it clear it's not variable (123 * u * s), but physical unit (123 in μs). In Japan, some community use "use square bracket always for physical unit" habit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I prefer fixed unit. It makes post analysis (e.g. generate Flame Graph) easy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed with @methane. Mixed units are confusing when values are meant to be compared. |
||
} | ||
|
||
if (mod == NULL) { | ||
goto error; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, maybe rename the option to "-X importime"? I don't know.
Maybe explain that the displayed time is the cumulative time, including import time of sub-imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed importprofile to importtime.
And self time is shown now, as Serhiy's suggestion.