bpo-45786: Allocate space for frame in frame object. #29729
Conversation
…bject outlive activation. WORK IN PROGRESS.
…bject outlive activation. WORK IN PROGRESS.
|
May even be a little bit faster |
markshannon
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @markshannon for commit af4933e 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
All the buildbot failures seemed to be from the deepfreeze build step.
So this PR appears to be good. |
| @@ -93,7 +93,6 @@ struct _Py_unicode_state { | |||
| # define PyTuple_MAXFREELIST 1 | |||
| # define PyList_MAXFREELIST 0 | |||
| # define PyDict_MAXFREELIST 0 | |||
| # define PyFrame_MAXFREELIST 0 | |||
There was a problem hiding this comment.
Why are we removing the freelist? The commit doesn't explain anything for this
There was a problem hiding this comment.
The frameobjects are no longer a fixed size, so a free list wouldn't work.
We shouldn't be allocating many frameobjects anyway, they only get creating when an exception is raised, or explicitly requested.
There was a problem hiding this comment.
But this will happen when tracing/profiling, no? I suspect coverage runs for example will be slower due to this.
There was a problem hiding this comment.
Maybe, but freelists just don't work unless all objects are the same size.
There was a problem hiding this comment.
The performance of coverage is already terrible, it needs to be fixed properly. One freelist more or less isn't going to make a difference.
See python/peps#2070 for something like a proper fix.
There was a problem hiding this comment.
I am skeptical of this, we are talking about the freelist of the frame object, which under coverage is going to be triggered a lot. I would like data to back this assertion.
There was a problem hiding this comment.
Which assertion? That the performance of coverage is already terrible, or that we need to fix it? 🙂
There was a problem hiding this comment.
This assertion:
One freelist more or less isn't going to make a difference.
There was a problem hiding this comment.
I just want to land this having an idea of the impact, not to stop the PR for landing, just to be clear
This is a bugfix, not a performance PR. |
Precisely, I want to know that coverage.py is not going to be tremendously slow because of this bugfix |
|
coverage.py is already tremendously slow. The (relatively infrequent) creation of a frame is not the problem. This is definitely something we need to address soon, but not here and not right now, IMO. |
Avoids the need to perform another allocation when a frameobject outlives its activation, removing a awkward situation where we might end up with an invalid frameobject due to a MemoryError.
https://bugs.python.org/issue45786