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

Pass am empty dict instead of a f_globals reference to profilers. #1836

Merged
merged 1 commit into from Sep 1, 2017

Conversation

x3k6a2
Copy link
Contributor

@x3k6a2 x3k6a2 commented Aug 23, 2017

The code right now creates a frame containing a reference to f_globals in f_locals. This is because of
https://github.com/python/cpython/blob/2.7/Objects/frameobject.c#L732

If a profiler accesses frame.f_locals (from python) it will trigger
frame_getlocals https://github.com/python/cpython/blob/02e03672e6766b1da847b1635982a70346780744/Objects/frameobject.c#L56
PyFrame_FastToLocals https://github.com/python/cpython/blob/02e03672e6766b1da847b1635982a70346780744/Objects/frameobject.c#L867
which will call map_to_dict with (pseudocode)
map_to_dict(f.co_code.co_varnames, len(f.co_code.co_varnames), f.f_locals, f.f_localsplus, 0)
which will remove everything from f_locals which is also in f.co_code.co_varnames.
As f_locals == f_globals in the current setup that modifies the global namespace.

In our case we have a function

def foo(foo):
  pass

if a function call to this function is profiled the function will be removed from the global namespace (as it shares it's name with one of it's parameter names).

The code right now creates a frame containing a reference to f_globals in f_locals. This is because of
https://github.com/python/cpython/blob/2.7/Objects/frameobject.c#L732

If a profiler accesses frame.f_locals (from python) it will trigger 
frame_getlocals https://github.com/python/cpython/blob/02e03672e6766b1da847b1635982a70346780744/Objects/frameobject.c#L56
PyFrame_FastToLocals https://github.com/python/cpython/blob/02e03672e6766b1da847b1635982a70346780744/Objects/frameobject.c#L867
which will call map_to_dict with (pseudocode) map_to_dict(f.co_code.co_varnames, len(f.co_code.co_varnames), f.f_locals, f.f_localsplus, 0)  which will remove everything from f_locals which is also in f.co_code.co_varnames.
As f_locals == f_globals in the current setup that modifies the global namespace. 

In our case we have a function
def foo(foo):
  pass
if a function call to this function is profiled the function will be removed from the global namespace (as it shares it's name with one of it's parameter names).
@scoder
Copy link
Contributor

scoder commented Sep 1, 2017

Thanks. While this introduces a tiny slow-down, and it does not actually repair f_locals, I can see that it's better than what we currently have.

@scoder scoder merged commit 15c065c into cython:master Sep 1, 2017
scoder added a commit that referenced this pull request Sep 1, 2017
@scoder
Copy link
Contributor

scoder commented Sep 1, 2017

I think I found a better fix that avoids the dict creation if it's not used, by setting certain code object flags instead. But I failed to reproduce the actual problem with your short example. Meaning, there is no actual test for this, and it will probably break again at some point. Could you try to come up with a test case that reproducibly fails for you (before applying the fix) ? I added a test to line_trace.pyx, but I couldn't get it to fail.

Sign up for free to 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.

None yet

2 participants