-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-37961: tracemalloc: store the actual length of traceback #15545
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
Conversation
ca7a93c
to
52a90f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
52a90f3
to
cd7f84e
Compare
@vstinner I just saw your comment on the issue. I'll update this to also reduce the size of the already existing |
cd7f84e
to
403780f
Compare
653a8e2
to
455e165
Compare
That should be ready for review but I see @bedevere-bot might be confused with labels. Ping @vstinner? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In C, you chose "total_nframe" name. In Python, it's "nframe" in the API, and sometimes "length" in the code. I would prefer to always use the same name everywhere.
I like "total_nframe", but I don't have a strong preference :-)
455e165
to
ac946ae
Compare
@vstinner Updated with your comments :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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().
ac946ae
to
f593044
Compare
Here you go @vstinner :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall change now LGTM, but I have one suggestion about the tracemalloc.Traceback constructor which may simplify the code and leaves the API kind of backward compatible.
f593044
to
1c78fe8
Compare
Last update I hope 🤞 @vstinner :) |
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. 👍 |
1c78fe8
to
7e30a3a
Compare
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. |
7e30a3a
to
7e4af49
Compare
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
7e4af49
to
c3c24ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But the CI blocks the PR.
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.
c3c24ed
to
dc54ec0
Compare
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