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

sys.setprofile doesn't do c_call in all cases #126056

Open
itamarst opened this issue Oct 27, 2024 · 6 comments
Open

sys.setprofile doesn't do c_call in all cases #126056

itamarst opened this issue Oct 27, 2024 · 6 comments
Labels
type-feature A feature request or enhancement

Comments

@itamarst
Copy link

itamarst commented Oct 27, 2024

Bug report

Bug description:

import sys
import mmap

def prof(frame, event, arg):
    print(event, arg)

sys.setprofile(prof)

x = mmap.mmap(-1, 10_000)
open("/dev/null")

When run with Python 3.13 on Linux:

c_call <built-in function open>
call None
call None
return None
return None
c_return <built-in function open>
return None

No mention of mmap.mmap(), perhaps because it's a constructor of a type object.

(This is really an issue with the underlying C API, PyTrace_C_CALL is similarly not happening.)

As someone doing profiling, this means that any code running inside C type constructors is invisible, which is problematic and means I'm going to have to change how one of my profilers works.

CPython versions tested on:

3.12, 3.13

Operating systems tested on:

Linux

@itamarst itamarst added the type-bug An unexpected behavior, bug, or error label Oct 27, 2024
@gaogaotiantian
Copy link
Member

This behavior is consistent with previous versions, it's not new. So this would be a feature request, instead of a bug.

@gaogaotiantian gaogaotiantian added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Oct 28, 2024
@itamarst
Copy link
Author

Normal profiling catches MyKlass.__init__, so it's surprising that C types' constructors don't get profiled. So it's a bug from the perspective of what you would expect from the documented API. If bugs are limited to regressions, then yes, not really a bug.

@gaogaotiantian
Copy link
Member

Well, not entirely. The documentation states

arg is the C function object.

which means in C API, it can be safely casted to a PyCFunctionObject* and all the related APIs and fields will be accessible. mmap.mmap does not have that. It's a type that calls the C function at tp_new to allocate and initialize the object, as most of the objects do.

So, supporting a type call, which is a breaking behavior, will actually break a lot of the profiler/debugger code out there which assumes the argument is a PyCFunctionObject*.

@itamarst
Copy link
Author

Perhaps could be solved by adding a new PyTrace_C_OBJECT_NEW or something?

@gaogaotiantian
Copy link
Member

gaogaotiantian commented Oct 28, 2024

It would still be a breaking change. Not only on C API level, should we add a new type of event for sys.setprofile? Because the arg is still different. How would we provide the arg? There's no Python representable data structure.

I'm not saying this can't or shouldn't be done. I'm simply stating that this is not a trivial bug fix. This is a new feature request that could potentially break plenty of the existing code, no matter how you implement it, because it breaks some assumptions that have been there for 10 years. Therefore, we'd better have a really good reason to add it.

@itamarst
Copy link
Author

The reason I encountered this was in a memory profiler: I use PyTrace_C_CALL to record and cache the latest line number, so that C code that releases the GIL and allocates can still record the the latest Python line number for that allocation. But e.g. mmap.mmap() is invisible to the profiler, so the cached line number is out of date and the allocation gets assigned to unrelated earlier line numbers.

More broadly, having certain code paths just be invisible to Python's profiling API is likely to hit other profilers based on these APIs. E.g. a C type constructor that does a lot of compute work will get assigned to the calling function and it will be difficult for profiler users to track down where exactly the slowness is.

Given it's not in current Python I will have to fix things regardless of what you all decide about this issue. Some alternatives I'm considering:

  • PyEval_SetProfile(), but per-line profiling is a lot more performance overhead, and you lose some info so I will have to figure out some failing tests caused by this change.
  • On every allocation, re-acquire the GIL so as to access the current frame. Slow in some cases (would hit every malloc), could cause deadlocks in some code.
  • Mucking around in internal frame implementation details, without holding the GIL. I and others have done this in different ways in different tools, but it's not fun. Needs extra work to support every CPython release, and may get increasingly more difficult as implementation shifts, the new internal frame representation was annoying to handle. And need to be very careful since it's all unstable and internal.
  • ... no doubt other approaches are possible ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants