Roofline Compare

[drchen-amd]

Controller

• Adjusted data generation range for roofline ceilings.

View

• Baseline Comparison
  • Moved metric tables from ComputeComparisonView to ComparisonTable. ComputeComparisonView now only a container.
  • Adjusted ComparisonTable padding, redistributed toolbar elements.
  • Added Roofline instance.
• Roofline
  • Added compare mode.
  • Added double click to reset zoom/pan.
  • Updated marker scheme for kernel intensities.
  • Consolidated handling of presets (1st drop down) and filters (2nd, 3rd, 4th dropdowns).
  • Allowed filters to become "custom" individually instead of all at once.
  • Added persistence for active filters upon switching workloads when possible.
  • Added optional basic on-chart labels for ceilings.
  • Improved handling for unexpected values (NaN, inf...etc)
• Limited workload and kernel selection dropdown widths via eliding.

(Protoype version with ceiling shading)

[amokiche-amd]

This class has a lot of fields and some of them alredy grouped by the comments. May be its time to add some substructures or subclasses to groop these class members?

[amokiche-amd]

Using a pointer to poiner here is porentialy high to debug crash: between the Update() call and the Render() call , something can trigger the model to destroy or rebuild its workload data: the user reopens a file, a background loader finishes, the model is reset.

[drchen-amd]

Can you clarify what you mean by "a background loader finishes"? Do you mean the initial loading? Or dynamic fetching of metrics?

I don't see this being affected by "user reopens a file" or those loading tasks. The model WorkloadInfos/KernelInfos objects I'm borrowing here are intended for persistent data. They are loaded once on file open, and this view does not read anything until said loading is complete. They are also stored in maps, so hypothetically if you added workloads/kernels after the fact, the existing ones have stable addresses. The dynamic live fetched model data (metrics) are kept separately, and Roofline does not care about them.

Yes if "the model is reset" mid-session, then this would probably crash, but I don't think this is an urgent scenario to handle today.

[amokiche-amd]

I agree that it’s unlikely to be feasible right now; the question is more about potential fragility down the line. I should have marked the comment as optional, you can marked it as resolved

[amokiche-amd]

This is why I recommend using smart pointers more often. You’ve added all the checks above:

bool ceiling_empty = !(m_ceiling_source && *m_ceiling_source) ||
(*m_ceiling_source)->roofline.ceiling_bandwidth.empty() ||
(*m_ceiling_source)->roofline.ceiling_compute.empty();

But here you’re only checking the outer pointer. The model could change the workload in the meantime, so this isn’t safe.

[drchen-amd]

Can you elaborate how smart pointers would help in this case? Smart pointers or not; I think these checks you've called out would remain as is.

As I mentioned in the previous comment, the model workload objects are fixed persistent data. If you're referring to surviving a workload being overwritten or removed, then we probably need an event driven dirty and update scheme, in which case the decision to use smart pointer or not seems tangential.

[amokiche-amd]

For example we can eddit the GetWorkloads to return the weak_ptr
std::weak_ptr<const WorkloadInfo> ComputeDataModel::GetWorkload(uint32_t workload_id) const

than replace raw pointer as
std::weak_ptr<const WorkloadInfo> m_workload_primary_weak; std::weak_ptr<const WorkloadInfo> m_workload_secondary_weak; std::shared_ptr<const WorkloadInfo> m_ceiling_source;

then just lock this weak pointers in the Update() or Render() and to assign the m_ceiling_source you can just assign shared_ptr to avoid pointer to pointer. Also If something clear it in the data model we still will have a valid shared_ptrs in all scope where we work with it.

It just concept and should be validated in the code, but I guess it make code cleaner, changes will eliminate the pointer-to-pointer and make the pointers safe during the execution of Render and Update.

[drchen-amd]

Okay so m_ceiling_source becomes shared_ptr insead of raw, in which case this check you're referring to:

bool ceiling_empty = !(m_ceiling_source && *m_ceiling_source) ||
(*m_ceiling_source)->roofline.ceiling_bandwidth.empty() ||
(*m_ceiling_source)->roofline.ceiling_compute.empty();

becomes this?

bool ceiling_empty = !m_ceiling_source ||
m_ceiling_source->roofline.ceiling_bandwidth.empty() ||
m_ceiling_source->roofline.ceiling_compute.empty();

This doesn't seem like a noteworthy difference to me.

Sure; if this view shares ownership of the model objects, it is safe in terms of the model not being able to pull the data out from underneath the view. However, without the dirty event + update scheme I mentioned, view will hold stale data in the scenario that model side changes. Smart_ptr or not, the model side changing mid-session is not handled.

[amokiche-amd]

I didn't mean that this would eliminate the need for such checks entirely; primarily, my proposal removes the need to check a double pointer. And yes, this approach won't protect against the data in the model changing "somewhere in the background." However, it will prevent a crash if the data in the model happens to be deleted for some reason. With raw pointers, we’d be left with a pointer to garbage data and potentialy undefined behavour. With smart pointers, we’d still have a pointer to the old data in the current frame, but in the next frame, we’d realize the data is gone and be able to handle the situation.
I’m not insisting on this implementation—it just strikes me as safer and cleaner—but the decision is yours.

[amokiche-amd]

I couldn't find a code path where kernel_changed wouldn't be set when the workload changes, but relying on this feels very fragile; we have to constantly keep it in mind and manually set all the flags, making it very easy to overlook such a path in the future. This comment isn't directed specifically at this PR, but rather reflects a general thought that we should find a simpler way to track changes to workloads and kernels.
This comment up to you, you can resolve it if don't agree.

[drchen-amd]

Cosmetic-only rework: Outer scroll view -> sub page for each widget.

[tomk-amd]

Certain kernel points are clipped form initial view when selecting a new kernel.

have to zoom out to see them:

Might be due to this finding?

Per-kernel bounds never refit the plot

SetupAxisLimits defaults to ImPlotCond_Once. The plot ID is the ceiling-source workload, so switching kernel or compare target keeps the first window. Constraints update; the view does not. After click-to-focus, ImPlot’s double-click fit can recover via the unfitted corner hint scatters — but only while nav is unlocked. Do not pass ImPlotCond_Always permanently; ImPlot locks the axes in that mode.

Reset limits (one-shot Always, or SetNextAxesLimits) when the (workload, kernel, compare-target) identity changes.

[tomk-amd]

Ceiling fetch errors are ignored, and ceiling values not retrieved if the last kernel intensity fetch fails

File: src/controller/src/compute/rocprofvis_controller_trace_compute.cpp:1109

On main, this was dm_result = ExecuteQuery(...). The new second pass drops the assignment:

query_arguments = { {kRPVComputeParamWorkloadId, std::to_string(id)} };
query_output = { {}, {} };
future->ResetProgress();
ExecuteQuery(db, m_dm_handle, object2wait, nullptr,
             kRPVComputeFetchWorkloadRooflineCeiling, query_arguments,
             query_output, [&roofline, &uint_data, &roofline_max_intensity_x,
                            &roofline_min_intensity_x](const QueryDataStore& data_store) {

If intensities load and the ceiling query fails, LoadRocpd still reports success. The view then treats the workload as empty (ceiling_bandwidth / ceiling_compute empty → “No data available”).

The ceiling pass is also gated on dm_result from the last intensity query, with no break on a failed kernel query. One failed last intensity query skips ceilings for every workload.

Ex:

The intensity loop overwrites dm_result on every kernel, and never breaks on failure:

rocprofvis_controller_trace_compute.cpp: Ln 1052–1088

    dm_result = ExecuteQuery(..., kRPVComputeFetchKernelRooflineIntensities, ...);
}
workload->SetObject(kRPVControllerWorkloadRoofline, 0, (rocprofvis_handle_t*)roofline);

After all workloads finish, ceilings run only if that leftover value is still success:

rocprofvis_controller_trace_compute.cpp Ln 1093–1109

if(dm_result == kRocProfVisDmResultSuccess)
{
    for(size_t i = 0; i < m_workloads.size(); i++)
    {
        // ...
                ExecuteQuery(..., kRPVComputeFetchWorkloadRooflineCeiling, ...);

So dm_result at line 1093 is not “did intensities load overall?”. It is the result of the last kRPVComputeFetchKernelRooflineIntensities call in the last workload that entered the inner if at line 1034.

That means:

If the last kernel intensity query fails, no workload gets ceilings — including ones whose intensities already succeeded and already had a Roofline object attached at line 1089.
If an earlier kernel fails and a later one succeeds, dm_result is success again, so a partial intensity load still opens the ceiling pass.

That if is also why restoring dm_result = ExecuteQuery(...) on the ceiling call is not enough by itself: a failed last intensity query never reaches the ceiling ExecuteQuery at all.

On main, ceiling fetch lived inside the per-workload intensity block, so a failed last kernel in workload A did not skip ceilings for workload B. Hoisting ceilings into a second pass tied them to one shared dm_result

AI Review

[drchen-amd]

ceiling values not retrieved if the last kernel intensity fetch fails
Yeah I think bot suggestion makes sense. The ceilings can just be a separate pass that that does not care about the intensity results at all. UI checks with individually anyway for emptiness.

Ceiling fetch errors are ignored:
This funny because before going through review with my bot, I had the loop break and report result if one failed. My bot didn't like that one failed ceiling meant all ceilings stopped loading, so I ended with this version where the result is not checked period. I don't know which is better. Maybe ceiling loop can keep a shared result like your bot says, but also not break if fail happens.

[tomk-amd]

\_( )_/ Ceiling fetch errors are ignored - I'll leave this to your judgement, maybe just log a warning if this happens?

this is the one that I think should be addressed: ceiling values not retrieved if the last kernel intensity fetch fails

[drchen-amd]

I've looked into these two and they are both moot due to current "all or nothing" loading scheme.

If ComputeTrace::LoadRocpd returns anything other than success, UI DP::ProcessLoadComputeTrace will not attempt to read anything from controller, so having error tolerance will not get us anything.

ceiling values not retrieved if the last kernel intensity fetch fails
Assuming the last kernel errors and ceilings loaded fine independently of the intensities; controller load will still return error, meaning UI will not load from controller.

Ceiling fetch errors are ignored:
Retaining the error will mean controller load will return error if a ceiling fails to load, which will cause UI to not load from controller.

[tomk-amd]

The return value for the Query can still be processed, for example logged, but not passed further down to affect the return value of the LoadRocpd().. So we can ignore the return value in the sense that it will not affect the rest of the flow but the failure will be logged so if some part of the roofline chart is missing- there will be an explanation as to why.. a "fault tolerant" design..

Perhaps this should be done for all Query results whose failures can be tolerated.. for example for the kernel intensity query, as long at least one is successful then the show can go on ?

[drchen-amd]

Perhaps this should be done for all Query results whose failures can be tolerated.

Yes I think this is what needs to happen to load the mostly empty pc sampling dbs, but are you saying you would like to see error handling reworked for all query results as a part of this change?

I could make the roofline parts consume their results and not emit externally, but if another component errors on load, the result will still be same, so I don't think it makes a difference unless every query is updated as you said.

[tomk-amd]

no - not as part of this change but as a general approach. We should do it via a targeted PR.