Community
Participate
Working Groups
It looks like this method was intended to return weither the debug session selected has changed, but it always returns true. It seems that a consequence of this is that the model will be rebuilt uselessly relatively often.
I have updated the title of this bug to better reflect what I think is the core of the issue. The previous title "setDebugSession() always return true" is part of the problem but not the whole of it. Here are a few observations: ATM the multicore visualizer model is recreated each time: - an applicable workbench selection happens (from the debug view) - each time the debug model changes, which can happen quite often and multiple times per debug command Launching a debug session with a small test program, with the multicore visualizer open, I count that the visualizer model is recreated about 25 times. It's then recreated 3 times each time I step into the program. Ideally, when debugging a single program, the visualizer model would only be created once and then just updated to reflect the state of the threads and the cores they are currently on.
Agreed -- for the initial version we elected to just re-create the model to keep things simple. In the update() method there's a TODO reminder that we do want to cache the model information and re-use it, if it hasn't changed.
I will soon submit a patch I have been working-on, that cuts down the number of visualizer model creations. Here are the main ideas: 1) workbench selection : will trigger a visualizer model re-creation only if the debug session has changed. 2) debug model change: will no longer trigger a visualizer model re-creation. will only trigger a check to see if the current debug session has changed, and if so, register the new session's debug listener. This is only to cover a small corner case where workbench selections events are not caught if the visualizer is hidden, and so might miss a newly started debug session. 3) a new debug event is listened-to: DataModelInitializedEvent. When received, it means that the debug model is finished building and the visualizer model can be created (i.e. the debug service is ready to answer questions about CPUs, cores, threads). 4) the debug event listener for event ISuspendedDMEvent, received when a thread stops, had been enhanced: it now also updates the core the thread is on, in addition to the thread's state. Previously we were relying on the model being reconstructed to re-assess the location of stopped threads. 5) a protection was added to prevent a race condition that became apparent with the above changes: a thread could be added twice to the model; once at model creation and a second time soon after through the listener. A check is now made in both places. 6) another little problem that became visible with the above changes is that no canvas refresh was triggered to reflect a visualizer selection change. It's a one-liner fix, so I did not think it was worth it to open a separate bug for this? Not addressed in this patch is the fact that the visualizer model needs to be re-created each time we change the selected debug session.
I have pushed the patch: https://git.eclipse.org/r/11601 w/r to the patch submitted in this bug, I declare the following: 1) that I am authorized to submit this code change 2) that the code is 100% my own work Marc Dumais
Hi, A small refresh issue was discovered, introduced by this patch: When turning off the load meters, the canvas would not be refreshed most of the time and so would still show the meters. EnableLoadMeterAction.run() would call MulticoreVisualizer.refresh() in this scenario. This previously worked because a re-creation of the model was triggered, refreshing the canvas afterwards. Looking at this scenario, it came to me that MulticoreVisualizer.refresh() no longer was doing what its name was advertising. So I propose the following small refactoring : 1) that what's in refresh() and move into workbenchSelectionChanged(), replacing the call to refresh(). 2) in refresh(), do an actual refresh of the canvas, recreating the graphical objects to match current model. I will shortly push a new version of the patch that contains this change. Bill, I would appreciate if you could have a look and confirm that this makes sense to you too. Regards, Marc
(In reply to comment #5) > 2) in refresh(), do an actual refresh of the canvas, recreating the > graphical objects to match current model. This change has a good side-effect, which is that after selecting the Refresh command from the Multicore Vis context menu, the selection is restored. It wasn't before. Nice!
(In reply to comment #5) > Hi, > > So I propose the following small refactoring : > 1) that what's in refresh() and move into workbenchSelectionChanged(), > replacing the call to refresh(). > 2) in refresh(), do an actual refresh of the canvas, recreating the > graphical objects to match current model. > > I will shortly push a new version of the patch that contains this change. > > Bill, I would appreciate if you could have a look and confirm that this > makes sense to you too. This does makes sense; refresh() is meant to just refresh the view from the model, but before we were having to use it to ping for changes as well.
Merged to master from Gerrit. Thanks Marc D for this.
