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
DM-10608: optimise Config history #17
Conversation
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.
Looks OK, few minor comments
python/lsst/pex/config/callStack.py
Outdated
return StackFrame.fromFrame(frame) | ||
|
||
|
||
class StackFrame(object): |
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 think object
needs from builtins import object
in Python3
python/lsst/pex/config/callStack.py
Outdated
import inspect | ||
import linecache | ||
|
||
__all__ = ['getStackFrame', 'StackFrame', 'getCallStack'] |
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.
This should be before regular imports but after __future__
import
python/lsst/pex/config/callStack.py
Outdated
""" | ||
frame = inspect.currentframe().f_back.f_back # Our caller's caller | ||
for ii in range(relative): | ||
frame = frame.f_back |
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.
You may want to add check for None
here in case someone passes too large relative
value by mistake
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.
Sorry, neglected to address this before merging.
I deliberately didn't add the None
check, so as to keep the code simple for optimal performance (this gets called tens of millions of times in a ci_hsc run), and because it's not necessary: if we go too far up the stack, an exception will automatically result as we attempt to access an attribute on None
, and it should then be very clear what the problem is.
python/lsst/pex/config/callStack.py
Outdated
""" | ||
frame = inspect.currentframe().f_back # Caller frame | ||
for ii in range(skip): | ||
frame = frame.f_back |
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.
Same here, may be worth protecting against too large skip
values.
traceback.extract_stack formats the traceback, including reading the source file. When doing a large Config manipulation, this adds a lot of unnecessary i/o (8% of a recent ci_hsc run) since that information isn't used except for debugging (and most information is immediately tossed anyway in order to get just the caller). Replaced traceback.extract_stack with a leaner implementation that only reads the source file when necessary. Added new interface that provides information for just the caller rather than getting the full stack trace and then throwing away all but one element.
Compiling and executing a regex is unnecessary when all we want to do is extract a trailing sub-string. This may save up to 6% of a total ci_hsc run.
We don't need to trace the full stack just to get the module of interest.
The wrapper for Control objects notes that mucking with the module is necessary for pickling. Since I've just changed the part that identifies the module, I thought a test that the pickling still works would be a good idea.
84a6b74
to
07e5312
Compare
No description provided.