Stewori / pytypes

Typing-toolbox for Python 3 _and_ 2.7 w.r.t. PEP 484.
Apache License 2.0
200 stars 20 forks source link

Inspect stack frame by frame to improve performance #107

Closed rover-debug closed 2 years ago

rover-debug commented 2 years ago

Addressing performance issue with the workaround used to address this issue: https://github.com/python/typing/issues/658.

Led to roughly 10x speedup in our project.

Stewori commented 2 years ago

If I understand it correctly, this change inverts the search order of the stack. Is that right? Probably this does not make a difference for the result. But programming is full of subtle problems. If it passes the tests I am fine with it.

Stewori commented 2 years ago

@bhavints Thank you for contributing this improvement!

rover-debug commented 2 years ago

If I understand it correctly, this change inverts the search order of the stack. Is that right? Probably this does not make a difference for the result. But programming is full of subtle problems. If it passes the tests I am fine with it.

Yes, the idea is that instead of collecting the entire stack via stack() and iterating through it, it is more performant to iterate backwards frame by frame until the class is found.

mitar commented 2 years ago

If I understand it correctly, this change inverts the search order of the stack.

No, the order is the same. stack() returns:

The first entry in the returned list represents the caller; the last entry represents the outermost call on the stack.

So calling currentframe() and then using f_back is walking in the same order. The difference is that it stops walking after you find the frame instead of first walking the whole stack.

mitar commented 2 years ago

@bhavints please also note of this. I suggest you make sure for frame to be deleted explicitly:

def handle_stackframe_without_leak():
    frame = inspect.currentframe()
    try:
        # do something with the frame
    finally:
        del frame
Stewori commented 2 years ago

If I understand it correctly while frame.f_back.f_back would skip the last two frames instead of the first two. Shouldn't rather the initialization change to frame = currentframe().f_back.f_back while the loop stays like before? As I understand @mitar currentframe() is stack[0].

mitar commented 2 years ago

Shouldn't rather the initialization change to frame = currentframe().f_back.f_back while the loop stays like before?

Yes, this is what I thought I am proposing. So the initialization should be done like you said. And then while frame.

rover-debug commented 2 years ago

OK thanks for the feedback, made some more edits.

mitar commented 2 years ago

Hm, CI tests have not run?

Stewori commented 2 years ago

Thank you for following all our comments patiently. And for moving the comment back to its proper location! Given also mitar approved I think this can be merged right away.

Stewori commented 2 years ago

Hm, CI tests have not run?

Yes, that puzzles me too. Is it not triggered by commits from new contributors?

mitar commented 2 years ago

I think you should first merge https://github.com/Stewori/pytypes/pull/108 and then ask @bhavints to rebase.