bpo-37961: tracemalloc: store the actual length of traceback #15545
Conversation
You should also update Doc/library/tracemalloc.rst documentation, document Trace.length new attribute (I suggest to add Traceback.total_nframe attribute instead):
https://docs.python.org/dev/library/tracemalloc.html#trace
Is it possible to load a pickle file created by Python 3.7 on Python 3.9 with this change?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
No, it failed with: I fixed it by setting a default |
|
@vstinner I just saw your comment on the issue. I'll update this to also reduce the size of the already existing |
jd
force-pushed
the
tracemalloc-nframes
branch
2 times, most recently
from
653a8e2
to
455e165
Compare
|
That should be ready for review but I see @bedevere-bot might be confused with labels. Ping @vstinner? |
|
@vstinner Updated with your comments :) |
Thanks, the updated PR looks better. Another round of minor comments.
You may mention total_nfrace in start() documentation: https://docs.python.org/dev/library/tracemalloc.html#tracemalloc.start Users may want to use it to adjust the next call to start().
|
Here you go @vstinner :) |
|
Last update I hope |
I'm not sure that I understand your comment. Do you suggest to modify your PR to allow total_nframe to be None? I think that I'm fine with that. _group_by() and loading a Python 3.8 file would use total_nframe=None if I understand correctly. In that case, I suggest to modify Traceback.repr() to omit "total_nframe=%s" if it's None. By the way, "<Traceback %r total_nframe=%s>" may be better to first show the most important first (the frames). So "<Traceback %r total_nframe=%s>" or "<Traceback %r>" depending if total_nframe is None. In your implementation, total_nframe is lost in tracemalloc.get_object_traceback(). I'm not sure if it's a deliberate choice or not. Maybe tracemalloc_get_traceback() should be modified to read a trace instead, and _tracemalloc__get_object_traceback() should use trace_to_pyobject() rather than traceback_to_pyobject(). But other functions using tracemalloc_get_traceback() should still return only the traceback. If you want, I can write this complex change, once this PR lands. |
You were suggesting to always set
Why not, done.
No, it's not. It's just a "feature not implemented yet". :)
Yeah, I think it'd be safer to go in a new PR that leverage this one rather than making this one heavier. |
In your PR, total_nframe is always set to an integer. When you don't know the value, you put len(frames), but you do that in the Traceback caller. I suggest to do that in the constructor to simplify your PR: tests, get_object_traceback(), _group_by(), etc. would just call Traceback(frames).
Again, your PR always set total_nframe to a number, so I don't understand your remark. Do you plan to modify your PR to set total_nframe to None in some cases (ex: _group_by())? If yes, you should update the documentation to mention that total_nframe can be None. |
That's mainly because the argument was mandatory in an earlier version IIRC. I can just remove it from the tests now if you prefer.
The initial plan was to always have it set to the number of frames — remember, it was named Now, the way the PR has changed, |
The initial plan was to always have it set to the number of frames — remember, it was named nframes. So it would either be the number of frames we captured or — if available — the number of total frames that were originally in place. So None was not possible value.
Now, the way the PR has changed, None could be a value. I've updated the PR in that sense. Some tests now uses None has a value. I've updated the attribute doc.
Oh, I see, the PR evolved and then was longer consistent. I'm fine with having total_nframe=None. It's just that it wasn't clear if it could be None or not. It's better with your update PR.
New review.
This adds a new field to the traceback_t data structure so it stores the original length of the traceback that was recorded. This is useful to know if the traceback has been truncated or not.
|
Thanks, fixed the doc! |
|
Add a total_nframe field to the traces collected by the tracemalloc module. This field indicates the original number of frames before it was truncated.
Add a total_nframe field to the traces collected by the tracemalloc module. This field indicates the original number of frames before it was truncated.
bpo-37961: tracemalloc: store the actual length of traceback
This adds a new field to the traceback_t data structure so it stores the
original length of the traceback that was recorded. This is useful to know if
the traceback has been truncated or not.
https://bugs.python.org/issue37961