| Message ID | 20230502092339.27341-1-mads@ynddal.dk (mailing list archive) |
|---|---|
| Headers | show |
| Series | simpletrace: refactor and general improvements | expand |
On Tue, May 2, 2023, 5:24 AM Mads Ynddal <mads@ynddal.dk> wrote: > From: Mads Ynddal <m.ynddal@samsung.com> > > I wanted to use simpletrace.py for an internal project, so I tried to > update > and polish the code. Some of the commits resolve specific issues, while > some > are more subjective. > > I've tried to divide it into commits so we can discuss the > individual changes, and I'm ready to pull things out, if it isn't needed. > Just a quick note to say that I'll be on and off the rest of this week, but this is on my radar to look at soon. A question for you: do you think it's possible to move simpletrace into qemu/python/utils? This requires cleaning up the code to some fairly pedantic standards, but helps protect it against rot as we change target python versions. No problem if that's too much to ask. just want to know if you had looked into it. > v2: > * Added myself as maintainer of simpletrace.py > * Improve docstring on `process` > * Changed call to `process` in scripts/analyse-locks-simpletrace.py to > reflect new argument types > * Replaced `iteritems()` with `items()` in > scripts/analyse-locks-simpletrace.py to support Python 3 > > Mads Ynddal (12): > simpletrace: Improve parsing of sys.argv; fix files never closed. > simpletrace: Annotate magic constants from QEMU code > simpletrace: changed naming of edict and idtoname to improve > readability > simpletrace: update code for Python 3.11 > simpletrace: Changed Analyzer class to become context-manager > simpletrace: Simplify construction of tracing methods > simpletrace: Improved error handling on struct unpack > simpletrace: define exception and add handling > simpletrace: Refactor to separate responsibilities > MAINTAINERS: add maintainer of simpletrace.py > scripts/analyse-locks-simpletrace.py: changed iteritems() to items() > scripts/analyse-locks-simpletrace.py: reflect changes to process in > simpletrace.py > > MAINTAINERS | 8 +- > scripts/analyse-locks-simpletrace.py | 5 +- > scripts/simpletrace.py | 307 ++++++++++++--------------- > 3 files changed, 150 insertions(+), 170 deletions(-) > > -- > 2.38.1 > > >
On Tue, May 02, 2023 at 11:23:27AM +0200, Mads Ynddal wrote: > From: Mads Ynddal <m.ynddal@samsung.com> > > I wanted to use simpletrace.py for an internal project, so I tried to update > and polish the code. Some of the commits resolve specific issues, while some > are more subjective. An internal project based on qemu.git or a completely separate codebase? Sometimes I've wondered whether tracetool should be extracted from qemu.git and moved into its own QEMU-independent place. That way other C/C++ applications and libraries could use it easily. Now that Alex Bennee removed the vcpu trace events that were specific to QEMU, the tracing code is less tightly coupled to QEMU. There are probably still a number of places that need to be cleaned up in order for the tracing code to be independent of QEMU though. If there is interest in doing this, I support the effort, although I'm not sure how much time I have to actually do the work myself. > > I've tried to divide it into commits so we can discuss the > individual changes, and I'm ready to pull things out, if it isn't needed. > > v2: > * Added myself as maintainer of simpletrace.py > * Improve docstring on `process` > * Changed call to `process` in scripts/analyse-locks-simpletrace.py to reflect new argument types > * Replaced `iteritems()` with `items()` in scripts/analyse-locks-simpletrace.py to support Python 3 > > Mads Ynddal (12): > simpletrace: Improve parsing of sys.argv; fix files never closed. > simpletrace: Annotate magic constants from QEMU code > simpletrace: changed naming of edict and idtoname to improve > readability > simpletrace: update code for Python 3.11 > simpletrace: Changed Analyzer class to become context-manager > simpletrace: Simplify construction of tracing methods > simpletrace: Improved error handling on struct unpack > simpletrace: define exception and add handling > simpletrace: Refactor to separate responsibilities > MAINTAINERS: add maintainer of simpletrace.py > scripts/analyse-locks-simpletrace.py: changed iteritems() to items() > scripts/analyse-locks-simpletrace.py: reflect changes to process in > simpletrace.py > > MAINTAINERS | 8 +- > scripts/analyse-locks-simpletrace.py | 5 +- > scripts/simpletrace.py | 307 ++++++++++++--------------- > 3 files changed, 150 insertions(+), 170 deletions(-) > > -- > 2.38.1 >
On Thu, May 4, 2023, 1:48 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Tue, May 02, 2023 at 11:23:27AM +0200, Mads Ynddal wrote: > > From: Mads Ynddal <m.ynddal@samsung.com> > > > > I wanted to use simpletrace.py for an internal project, so I tried to > update > > and polish the code. Some of the commits resolve specific issues, while > some > > are more subjective. > > An internal project based on qemu.git or a completely separate codebase? > > Sometimes I've wondered whether tracetool should be extracted from > qemu.git and moved into its own QEMU-independent place. That way other > C/C++ applications and libraries could use it easily. > > Now that Alex Bennee removed the vcpu trace events that were specific to > QEMU, the tracing code is less tightly coupled to QEMU. There are > probably still a number of places that need to be cleaned up in order > for the tracing code to be independent of QEMU though. > > If there is interest in doing this, I support the effort, although I'm > not sure how much time I have to actually do the work myself. > I meant internal, but if there's interest in fully extracting it, I have a playbook for that now based on my efforts to do the same for qemu.qmp and I can offer good stewardship for that process. I only suggest moving it under python/ so that we have it type checked and tested against python deprecations as part of CI as a preventative measure against rot. (Stuff in python/ is rigorously checked, stuff in scripts/ is not.) --js > > > > I've tried to divide it into commits so we can discuss the > > individual changes, and I'm ready to pull things out, if it isn't needed. > > > > v2: > > * Added myself as maintainer of simpletrace.py > > * Improve docstring on `process` > > * Changed call to `process` in scripts/analyse-locks-simpletrace.py to > reflect new argument types > > * Replaced `iteritems()` with `items()` in > scripts/analyse-locks-simpletrace.py to support Python 3 > > > > Mads Ynddal (12): > > simpletrace: Improve parsing of sys.argv; fix files never closed. > > simpletrace: Annotate magic constants from QEMU code > > simpletrace: changed naming of edict and idtoname to improve > > readability > > simpletrace: update code for Python 3.11 > > simpletrace: Changed Analyzer class to become context-manager > > simpletrace: Simplify construction of tracing methods > > simpletrace: Improved error handling on struct unpack > > simpletrace: define exception and add handling > > simpletrace: Refactor to separate responsibilities > > MAINTAINERS: add maintainer of simpletrace.py > > scripts/analyse-locks-simpletrace.py: changed iteritems() to items() > > scripts/analyse-locks-simpletrace.py: reflect changes to process in > > simpletrace.py > > > > MAINTAINERS | 8 +- > > scripts/analyse-locks-simpletrace.py | 5 +- > > scripts/simpletrace.py | 307 ++++++++++++--------------- > > 3 files changed, 150 insertions(+), 170 deletions(-) > > > > -- > > 2.38.1 > > >
> A question for you: do you think it's possible to move simpletrace into qemu/python/utils? This requires cleaning up the code to some fairly pedantic standards, but helps protect it against rot as we change target python versions. > > No problem if that's too much to ask. just want to know if you had looked into it. Potentially, this would be possible. But `simpletrace.py` imports `qemu/scripts/tracetool/`, so that would have to be moved as well, or installed as a package. I haven't touched the `tracetool` code itself, so I'm not sure how feasible it is (i.e. how many other places use `tracetool`).
On Thu, May 04, 2023 at 01:53:38PM -0400, John Snow wrote: > On Thu, May 4, 2023, 1:48 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Tue, May 02, 2023 at 11:23:27AM +0200, Mads Ynddal wrote: > > > From: Mads Ynddal <m.ynddal@samsung.com> > > > > > > I wanted to use simpletrace.py for an internal project, so I tried to > > update > > > and polish the code. Some of the commits resolve specific issues, while > > some > > > are more subjective. > > > > An internal project based on qemu.git or a completely separate codebase? > > > > Sometimes I've wondered whether tracetool should be extracted from > > qemu.git and moved into its own QEMU-independent place. That way other > > C/C++ applications and libraries could use it easily. > > > > Now that Alex Bennee removed the vcpu trace events that were specific to > > QEMU, the tracing code is less tightly coupled to QEMU. There are > > probably still a number of places that need to be cleaned up in order > > for the tracing code to be independent of QEMU though. > > > > If there is interest in doing this, I support the effort, although I'm > > not sure how much time I have to actually do the work myself. > > > > I meant internal, but if there's interest in fully extracting it, I have a > playbook for that now based on my efforts to do the same for qemu.qmp and I > can offer good stewardship for that process. I was curious how Mads is using simpletrace for an internal (to Samsung?) project. Maybe only simpletrace.py is needed because Mads' own code emits trace logs in the simpletrace binary format, but in the general case we could extract most of QEMU's tracing infrastructure (including tracetool). Stefan
On Mon, May 08, 2023 at 03:28:04PM +0200, Mads Ynddal wrote: > > > A question for you: do you think it's possible to move simpletrace into qemu/python/utils? This requires cleaning up the code to some fairly pedantic standards, but helps protect it against rot as we change target python versions. > > > > No problem if that's too much to ask. just want to know if you had looked into it. > > Potentially, this would be possible. But `simpletrace.py` imports > `qemu/scripts/tracetool/`, so that would have to be moved as well, or installed > as a package. I haven't touched the `tracetool` code itself, so I'm not sure how > feasible it is (i.e. how many other places use `tracetool`). tracetool is only used by QEMU's build system to generate code from the trace-events files. In theory it's possible to move it. I'm not sure I understand the purpose of moving it to python/. What infrastructure does python/ provide that's not available to simpletrace.py in its current location? Stefan
> > I was curious how Mads is using simpletrace for an internal (to > Samsung?) project. > I was just tracing the NVMe emulation to get some metrics. The code is all upstream or a part of this patchset. The rest is tracing configs.
On Mon, May 08, 2023 at 06:50:58PM +0200, Mads Ynddal wrote: > > > > > I was curious how Mads is using simpletrace for an internal (to > > Samsung?) project. > > > > I was just tracing the NVMe emulation to get some metrics. The code is all > upstream or a part of this patchset. The rest is tracing configs. I see, not a different codebase from QEMU. In that case what I said about extracting tracetool and simpletrace from qemu.git won't be useful. Stefan
On Mon, May 8, 2023 at 11:16 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Mon, May 08, 2023 at 03:28:04PM +0200, Mads Ynddal wrote: > > > > > A question for you: do you think it's possible to move simpletrace into qemu/python/utils? This requires cleaning up the code to some fairly pedantic standards, but helps protect it against rot as we change target python versions. > > > > > > No problem if that's too much to ask. just want to know if you had looked into it. > > > > Potentially, this would be possible. But `simpletrace.py` imports > > `qemu/scripts/tracetool/`, so that would have to be moved as well, or installed > > as a package. I haven't touched the `tracetool` code itself, so I'm not sure how > > feasible it is (i.e. how many other places use `tracetool`). > > tracetool is only used by QEMU's build system to generate code from the > trace-events files. In theory it's possible to move it. > > I'm not sure I understand the purpose of moving it to python/. What > infrastructure does python/ provide that's not available to > simpletrace.py in its current location? > > Stefan It's just directory-based: python/qemu/ are formatted as packages, with python/qemu/util/ serving as a package that houses a bag of scripts. You can install these things as a package into a venv and use them anywhere. code in python/ is checked with a linter, type checker, etc while code in scripts/ is not. It isn't installed anywhere and you may or may not have to carefully set up PYTHONPATH= or choose your CWD carefully to get the imports correct. Over time I want to move more python code over under python/ so it gets checked with the robots. The best stuff to move is stuff with imports and dependencies. Truly standalone scripts aren't as important to move. It's not important for today, so let's not worry about it for this series. --js
From: Mads Ynddal <m.ynddal@samsung.com> I wanted to use simpletrace.py for an internal project, so I tried to update and polish the code. Some of the commits resolve specific issues, while some are more subjective. I've tried to divide it into commits so we can discuss the individual changes, and I'm ready to pull things out, if it isn't needed. v2: * Added myself as maintainer of simpletrace.py * Improve docstring on `process` * Changed call to `process` in scripts/analyse-locks-simpletrace.py to reflect new argument types * Replaced `iteritems()` with `items()` in scripts/analyse-locks-simpletrace.py to support Python 3 Mads Ynddal (12): simpletrace: Improve parsing of sys.argv; fix files never closed. simpletrace: Annotate magic constants from QEMU code simpletrace: changed naming of edict and idtoname to improve readability simpletrace: update code for Python 3.11 simpletrace: Changed Analyzer class to become context-manager simpletrace: Simplify construction of tracing methods simpletrace: Improved error handling on struct unpack simpletrace: define exception and add handling simpletrace: Refactor to separate responsibilities MAINTAINERS: add maintainer of simpletrace.py scripts/analyse-locks-simpletrace.py: changed iteritems() to items() scripts/analyse-locks-simpletrace.py: reflect changes to process in simpletrace.py MAINTAINERS | 8 +- scripts/analyse-locks-simpletrace.py | 5 +- scripts/simpletrace.py | 307 ++++++++++++--------------- 3 files changed, 150 insertions(+), 170 deletions(-)