Bug 396268 - [visualizer] Add CPU/core load information to the multicore visualizer
Summary: [visualizer] Add CPU/core load information to the multicore visualizer
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: Next   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 8.2   Edit
Assignee: Marc Dumais CLA
QA Contact: Marc Khouzam CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-11 08:21 EST by Marc Dumais CLA
Modified: 2013-09-06 22:12 EDT (History)
4 users (show)

See Also:


Attachments
example of load display in the visualizer (7.11 KB, image/png)
2012-12-11 11:17 EST, Marc Dumais CLA
no flags Details
Another example, with 4 CPU each having 4 cores (18.54 KB, image/png)
2012-12-11 14:57 EST, Marc Dumais CLA
no flags Details
New version of the Visualizer, following Bill's comments. (5.43 KB, image/png)
2012-12-12 10:46 EST, Marc Dumais CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Dumais CLA 2012-12-11 08:21:29 EST
It would be interesting to enhance the multicore visualizer to show the current load of the CPU(s) / core(s).  It would also be necessary to implement the backend functionality so that the visualizer can request the load information.  

On Linux, a quick, reliable way to get the load is to take multiple readings of /proc/stat CPU counters and calculate the proportion of the increase that went in busy work.
Comment 1 Marc Dumais CLA 2012-12-11 11:17:50 EST
Created attachment 224577 [details]
example of load display in the visualizer

This is the prototype we have now; the actual load values are randomly genereated, just to illustrate the display part.  The high waternarks are also set at bogus values.
Comment 2 William Swanson CLA 2012-12-11 11:45:57 EST
This looks reasonable -- a couple suggestions:
- for consistency, the CPU bar should be drawn on the right
- The "meters" stand out a bit, perhaps more than they should;
  a couple things to try here:
  - make the background color of the meter the same as the current
    background color of the cpu/core, so it doesn't look like a "hole"
    in the background color
  - make the meter's edge color a little dimmer, again to differentiate
    it from the "edge" of the core/cpu
Comment 3 Marc Dumais CLA 2012-12-11 14:57:01 EST
Created attachment 224598 [details]
Another example, with 4 CPU each having 4 cores

again the actual load values are bogus.
Comment 4 Marc Dumais CLA 2012-12-12 08:19:28 EST
(In reply to comment #2)

Hi Bill,

Thanks; I will provide a new screenshot when this is done, with your comments taken into account!  

Regards,

Marc
Comment 5 Marc Dumais CLA 2012-12-12 10:46:29 EST
Created attachment 224633 [details]
New version of the Visualizer, following Bill's comments.
Comment 6 Marc Dumais CLA 2013-01-31 13:48:13 EST
Hi!

I pushed the proposed code change to implement this bugzilla item:

https://git.eclipse.org/r/#/c/10077/

Marc K. will do the review.  Bill and others you are welcome to join the review if you want/have time.

Regards,

Marc
Comment 7 Marc Dumais CLA 2013-01-31 13:48:32 EST
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
Comment 8 Marc Khouzam CLA 2013-01-31 15:15:00 EST
(In reply to comment #6)
> Hi!
> 
> I pushed the proposed code change to implement this bugzilla item:
> 
> https://git.eclipse.org/r/#/c/10077/

Thanks for that.

> Marc K. will do the review.  Bill and others you are welcome to join the
> review if you want/have time.

I was planning on reviewing it, but Bill's input would be valuable as well.

Once the contribution is accepted, I should be the one committing it so we can avoid having to go through the CR process, since Marc and I both work at the same place.
Comment 9 Marc Khouzam CLA 2013-02-11 21:07:39 EST
I've done a first review and things look good.  I've posted the comments on Gerrit.  Most everything is minor and should be easy to fix.

Thanks for the contribution.  I think once you address the comments, we'll be close to being able to commit it.
Comment 10 Marc Dumais CLA 2013-02-18 14:31:52 EST
So we do not forget...  

We had an internal discussion and decided to remove the high-load watermark from the service's API.   It seemed wrong to have the service take care of tracking the watermark when it's the view that controls when the measurements happen.  

For now the high-load watermark is not implemented in the view.
Comment 11 Marc Khouzam CLA 2013-02-18 16:09:30 EST
Looks good.

Below are general comment I had put in Gerrit that we forgot to discuss:

1- when I kill the visualizer view, and re-open it, it does not remember that I had enabled the load meters.  I think it should, no?  We'll need to store this setting in the preference store.  We can open a new bug for this.

2- how difficult would it be to draw the cores bigger if the load meters are not shown since we don't have to keep space free on the margin for the load of the CPU?

3- On my machine I cannot see what is selected for the load refresh speed, there are no checkmarks or anything

4- When I disable the load meters, the view does not refresh until I select something in the Debug View, so the load meter remain there
  When I enable, they only appear after the first timeout, which can seem a bit long.

5- Do we really want to be able to set the refresh speed even if the Load meters are disabled? It might simply things if that sub-sub-menu was disabled if the meters are disabled

6- Can we handle the case where getting the load returns an error in a more user-friendly way? Maybe a pop up using GdbStatusHandler and disabling the enable command?  We can open a new bug about this.

7- Say we cannot get the load for 5 times in a row, maybe we should disable the feature, and require the user to manually re-enable it? Or else we'll keep sending request to GDB I believe.  We can open a new bug about this.

8- I think having the load meter actions in the view menu may be more appropriate than the context menu. What do you think?

9- We should open a new bug to prevent repeating traces from being displayed in the gdb traces console.
Comment 12 Marc Khouzam CLA 2013-02-18 16:20:06 EST
10- when I click on the debug view (just re-selecting what is selected) the CPU bars disappear and then re-appear.  This does not happen with the rest of the visualizer.
Comment 13 Marc Dumais CLA 2013-02-22 11:29:46 EST
Salut Marc,

Thanks for the comments.  See below my responses (I will wait for the feature to be merged before opening bugs on it ) : 

1- when I kill the visualizer view, and re-open it, it does not remember that I had enabled the load meters.  I think it should, no?  We'll need to store this setting in the preference store.  We can open a new bug for this.
 -> will open bug when feature released

2- how difficult would it be to draw the cores bigger if the load meters are not shown since we don't have to keep space free on the margin for the load of the CPU?
 -> Done

3- On my machine I cannot see what is selected for the load refresh speed, there are no checkmarks or anything
 -> Done

4- When I disable the load meters, the view does not refresh until I select something in the Debug View, so the load meter remain there  When I enable, they only appear after the first timeout, which can seem a bit long.

 -> Added refresh when disabling the load meters.  Now when we disable the meters right-clicking on a core with no thread running, the refresh has the side-effect of showing a blank visualizer.  
 -> will open bug about this, as the users might not understand why the visualizer seems to disappears in this case.

5- Do we really want to be able to set the refresh speed even if the Load meters are disabled? It might simply things if that sub-sub-menu was disabled if the meters are disabled
 -> Done

6- Can we handle the case where getting the load returns an error in a more user-friendly way? Maybe a pop up using GdbStatusHandler and disabling the enable command?  We can open a new bug about this.
 -> will open bug when feature released

7- Say we cannot get the load for 5 times in a row, maybe we should disable the feature, and require the user to manually re-enable it? Or else we'll keep sending request to GDB I believe.  We can open a new bug about this.
-> will open bug when feature released


8- I think having the load meter actions in the view menu may be more appropriate than the context menu. What do you think?
-> ???  I am not sure I understand this one.  What is the view menu?  I see only the one menu in the code.


9- We should open a new bug to prevent repeating traces from being displayed in the gdb traces console.
-> will open bug when feature released
Comment 14 Marc Dumais CLA 2013-02-22 11:45:21 EST
10- when I click on the debug view (just re-selecting what is selected) the CPU bars disappear and then re-appear.  This does not happen with the rest of the visualizer.

-> I believe that this is a side-effect of "Bug 399419 -  MulticoreVisualizer:setDebugSession() always return true". Because of the bug, the visualizer wrongly concludes that the debug session has changed when you re-click on the same debug session and so the model is reconstructed.  It takes a little while to get load information.  Until the load information is available, the load meters are not shown, causing what you saw.
Comment 15 Marc Khouzam CLA 2013-02-22 16:17:40 EST
(In reply to comment #13)

Thanks.  The changes look good.

> 3- On my machine I cannot see what is selected for the load refresh speed,
> there are no checkmarks or anything
> -> Done

Would using radio buttons be better?

> 4- When I disable the load meters, the view does not refresh until I select
> something in the Debug View, so the load meter remain there  When I enable,
> they only appear after the first timeout, which can seem a bit long.
> 
>  -> Added refresh when disabling the load meters.  Now when we disable the
> meters right-clicking on a core with no thread running, the refresh has the
> side-effect of showing a blank visualizer.  
>  -> will open bug about this, as the users might not understand why the
> visualizer seems to disappears in this case.

I don't see this, or at least I didn't understand how to reproduce it.

> 8- I think having the load meter actions in the view menu may be more
> appropriate than the context menu. What do you think?
> -> ???  I am not sure I understand this one.  What is the view menu?  I see
> only the one menu in the code.

The Visualizer currently does not have a view menu since there's nothing in it.  The view menu is the upside-down triangle on the right-hand side of other views.

(In reply to comment #14)
> 10- when I click on the debug view (just re-selecting what is selected) the
> CPU bars disappear and then re-appear.  This does not happen with the rest
> of the visualizer.
> 
> -> I believe that this is a side-effect of "Bug 399419 - 
> MulticoreVisualizer:setDebugSession() always return true". Because of the
> bug, the visualizer wrongly concludes that the debug session has changed
> when you re-click on the same debug session and so the model is
> reconstructed.  

Ok, but there will be valid cases where the model needs to be updated.  So even if we fix this bug, we'll get this behavior once in a while.

> It takes a little while to get load information.  Until the
> load information is available, the load meters are not shown, causing what
> you saw.

How about if we don't wait for the load info to be available to show the CPU bars?  If the CPU bars are enabled, we should draw them even if we don't have their content yet.  But that probably will cause the load to drop to 0 while waiting for the refresh.  It's better but I fear it may still look weird.  We could also immediately draw the loads using whatever value we used to have, and trigger a refresh at the same time.  That should make a re-selection more smooth.

What do you think?
Comment 16 Marc Dumais CLA 2013-02-26 08:37:07 EST
(In reply to comment #15)
> 
> > 3- On my machine I cannot see what is selected for the load refresh speed,
> > there are no checkmarks or anything
> > -> Done
> 
> Would using radio buttons be better?
Yes, done.  Do you think I should use a checkbox for the enable/disable of the load meters?  
 
> (In reply to comment #14)
> > 10- when I click on the debug view (just re-selecting what is selected) the
> > CPU bars disappear and then re-appear.  This does not happen with the rest
> > of the visualizer.
> > 
> > -> I believe that this is a side-effect of "Bug 399419 - 
> > MulticoreVisualizer:setDebugSession() always return true". Because of the
> > bug, the visualizer wrongly concludes that the debug session has changed
> > when you re-click on the same debug session and so the model is
> > reconstructed.  
> 
> Ok, but there will be valid cases where the model needs to be updated.  So
> even if we fix this bug, we'll get this behavior once in a while.
> 
I have modified the feature so that the availability of load values no longer determines if the load meters are displayed.  Instead there is a switch in the model.  This will prevent the load meters from appearing and disappearing when the debug context is changed.   

> > It takes a little while to get load information.  Until the
> > load information is available, the load meters are not shown, causing what
> > you saw.
> 
> How about if we don't wait for the load info to be available to show the CPU
> bars?  If the CPU bars are enabled, we should draw them even if we don't
> have their content yet.  But that probably will cause the load to drop to 0
> while waiting for the refresh.  It's better but I fear it may still look
> weird.  We could also immediately draw the loads using whatever value we
> used to have, and trigger a refresh at the same time.  That should make a
> re-selection more smooth.

When we re-select the debug context, the model is reconstructed (bug 399419).  The old load values are gone with the old model.  When this bug is fixed, I think we can continue to show the old values until we get new ones.  In the meantime we need to show something. I am thinking of just showing zeros until we have actual values. 

I will upload the newest version of the patch later today.

Thanks for all the constructive comments!

Marc
Comment 17 Marc Khouzam CLA 2013-02-26 11:48:47 EST
(In reply to comment #16)
> (In reply to comment #15)
> > 
> > > 3- On my machine I cannot see what is selected for the load refresh speed,
> > > there are no checkmarks or anything
> > > -> Done
> > 
> > Would using radio buttons be better?
> Yes, done.  Do you think I should use a checkbox for the enable/disable of
> the load meters?  

That sounds good too.  I don't have a preference.  Up to you.
Comment 18 William Swanson CLA 2013-02-26 12:00:51 EST
(In reply to comment #17)
> > > 
> > > > 3- On my machine I cannot see what is selected for the load refresh speed,
> > > > there are no checkmarks or anything
> > > 
> > ... Do you think I should use a checkbox for the enable/disable of
> > the load meters?  
> 
> That sounds good too.  I don't have a preference.  Up to you.

In general, checkboxes are best for boolean on/off items,
and radio buttons for multiple-choice items.

> > 8- I think having the load meter actions in the view menu may be more
> > appropriate than the context menu. What do you think?

We should revisit this in looking at an overall "pluggable" design for
the Multicore Visualizer, since individual contributions might have
options/preferences to present and manage.

The view menu tends to be used for high-level preferences relating to
how the view arranges itself (Example: the Layout->Vertical/Horizontal
option on the Type Hierarchy and debugging views) and the context menu,
as the name implies, is used for actions performable based on where
you right-clicked (like attaching a debugger to a tile). So where should
a preference for the load meter go? Not sure. If you could, for example,
turn the meter on/off per tile, it might go in the context menu, but if
it's enabled/disabled for every tile, might be better on the view menu.
At some point we might need to have a separate Preferences page for the
visualizer to contain the various options, rather than overloading either
the view or the context menu.
Comment 19 Marc Khouzam CLA 2013-02-26 16:27:55 EST
(In reply to comment #18)
>  So where should
> a preference for the load meter go? Not sure. If you could, for example,
> turn the meter on/off per tile, it might go in the context menu, but if
> it's enabled/disabled for every tile, might be better on the view menu.
> At some point we might need to have a separate Preferences page for the
> visualizer to contain the various options, rather than overloading either
> the view or the context menu.

Yes, I agree that we will surely have to revisit these menus as we add more information to the Multicore Visualizer.

Until then, let's leave the CPU meter option in the context menu, to avoid extra work that may be thrown out anyway.
Comment 20 Marc Khouzam CLA 2013-02-28 11:13:33 EST
I've committed patch set 12 from Gerrit.

Thanks Marc D. for the hard-work and patience.

Can you please update the N&N with this nice feature?
http://wiki.eclipse.org/CDT/User/NewIn82
Comment 21 Marc Dumais CLA 2013-03-04 09:36:04 EST
(In reply to comment #20)
> I've committed patch set 12 from Gerrit.
> 
> Thanks Marc D. for the hard-work and patience.
> 
> Can you please update the N&N with this nice feature?
> http://wiki.eclipse.org/CDT/User/NewIn82

N&N updated.  Thanks!

Marc