-
-
Notifications
You must be signed in to change notification settings - Fork 874
Merge GpuEvent and ZoneEvent #1155
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
base: master
Are you sure you want to change the base?
Conversation
This patch changes GPU to use ZoneEvents, to reduce code duplication and allow consistent feature support between GPU and CPU. The CPU is given a context similar to how GPUs have a context. The following features now work with GPU events: Trace Comparison Zone Search Zone Histogram Flame Graph Fixes wolfpld#1109
9668fa5
to
7d9486f
Compare
It would be helpful if you'd describe changes made in |
The GpuEvent is removed and replaced with ZoneEvent. The extra fields of GpuEvent (the two extra timestamps, and query_id) are added to ZoneExtra. (This of course uses up extra bytes with CPU events, but that could be fixed I think). Replacing the GpuEvent with ZoneEvent allows a bunch of nearly identical functions to be consolidated. TracyWorker.cpp contains some parallel data structures for CPU and GPU, like sourceLocationZones and gpuSourceLocationZones. These data structures are moved to a new ZoneContext type, which is a base class for CPU and GPU. GpuCtxData is made a subclass of ZoneContext, and a new context type CPUZoneContext is added for CPU. Worker creates a default CPU context automatically. GpuSourceLocationZones and GpuZoneThreadData are redundant with non-gpu and are removed. The ZoneThreadData and SourceLocationZones are moved to TracyContext.hpp. Both contexts use the same ThreadData type now, but CPU has a subclass to store extra information for sampling data. The m_threadMap field serves the role that GpuCtxData::threadData does for CPU, so that got moved to the ZoneContext, along with m_data.threads, threadCtx, threadCtxData. Zone child data is still held by Worker. There is an assumption that the same allocation of ZoneEvent would never be in two contexts, so it should work. Also, all contexts share the original zoneExtra data storage in Worker. If that data structure were moved to contexts, the CPU version could avoid the extra GPU fields. Most of the remaining changes are refactoring to deal with the above changes. The UI is changed in some places to have a Combo box to select a context, and display the context with search results. There is no change to the protocol, but there is to the file format. |
I'm looking into creating a revision that only replaces GpuEvent with ZoneEvent and doesn't introduce a CPU context. I don't know yet if it will work. |
In order to to finish this, I have to engineer a bunch of alternate solutions to things I already solved, not sure if it's worth it. |
There's a lot to digest here, so let me see if I understand. The core idea here is to make GPU zones/events behave more like CPU zones/events (more like a full unification), and as such unlock zone utilities (search, compare, histograms, etc) for GPU zones? If so, I think this should be introduced/merged more incrementally (for example, leave changes around (Also: I think some of the work here can help with this issue/request: #1154) |
This patch changes GPU to use ZoneEvents, to reduce code duplication and allow consistent feature support between GPU and CPU. The CPU is given a context similar to how GPUs have a context.
The following features now work with GPU events:
Trace Comparison
Zone Search
Zone Histogram
Flame Graph
Fixes #1109
I'm posting this for some feedback on the concept. There are still some major to do items for this:
Also, this rework could allow more than one CPU context, which could be used to support cluster tracing.