After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 632188 - Not enough CPU usage line graph colors for high core count systems
Not enough CPU usage line graph colors for high core count systems
Status: RESOLVED FIXED
Product: system-monitor
Classification: Core
Component: resources
git master
Other Linux
: Normal normal
: ---
Assigned To: Chris Kühl
Depends on:
Blocks:
 
 
Reported: 2010-10-15 02:33 UTC by Luke Hutchison
Modified: 2012-01-28 01:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add support for more than four cpu graph colors (7.36 KB, patch)
2011-11-26 11:18 UTC, Johannes Pfeiffer
reviewed Details | Review
Change GSettings type for CPU colors to a(us) (7.62 KB, patch)
2012-01-08 18:16 UTC, Chris Kühl
accepted-commit_now Details | Review
Proposed patch (2.08 KB, patch)
2012-01-20 19:18 UTC, Robert Roth
none Details | Review

Description Luke Hutchison 2010-10-15 02:33:34 UTC
I use a 24-core Xeon system at work, and system-monitor only colors the first 4 CPU usage lines different colors, then the other 20 lines are all red.  Probably a random RGB triple should be generated for each line (with manually-picked aesthetic colors for the first 4 or 8 cores maybe)?
Comment 1 Luke Hutchison 2010-10-15 02:35:35 UTC
...or a better scheme, spiral inwards around the HSV wheel, fitting all colors into a spiral curve and either keeping the angular increment constant or varying the angle and arriving near the center of the color wheel after a fixed number of turns?
Comment 2 Philippe Chaintreuil 2011-10-17 20:34:11 UTC
I have a 16-core PC and have a similar problem.  All I see is a bunch of red lines.
Comment 3 Johannes Pfeiffer 2011-11-26 11:18:41 UTC
Created attachment 202177 [details] [review]
Add support for more than four cpu graph colors

Although I have not changed the way colors are selected for more than 4 cores, I have a small patch attached, which at least allows to store more than 4 colors for the cpu graphs.

It's my first time I changed something inside the gnome sources, so please let me know if this is a feasible way of doing things :-)
Comment 4 Robert Roth 2011-11-26 19:54:30 UTC
Review of attachment 202177 [details] [review]:

I like this patch, it seems to be clear and should work. The migration case however is not covered, that being the only inconvenience, so people who have already configured their first 4 CPU colors will have to reconfigure them, but I think they should be able to live with that, as migrating would mean to keep both cpu_color1-4 and cpu_colors around, and the code handling the migration.
Comment 5 Johannes Pfeiffer 2011-11-27 15:13:41 UTC
Thanks for the review, glad to hear the patch looks sane to you. I did not really think about migration when writing the patch, since I had no idea how to remove the cpu_color% properties from the schema once the migration was performed (and having these properties still lurking around was not the ideal solution for me). Or maybe I'm simply not understanding how settings are managed and there is a rather simple solution for this problem.

If you can point me to any direction, I'm glad to learn more (even if its not a 'must' have feature) :-)
Comment 6 Chris Kühl 2012-01-08 18:16:05 UTC
Created attachment 204835 [details] [review]
Change GSettings type for CPU colors to a(us)

I've actually implemented this a bit differently. Instead of a simple string I'm using a GVariant  of type a(us) where the u is the CPU index and the string is the color. This is type safe, avoids the string manipulation and associated the color with a CPU index.

I'm not going to worry about the migration since it is likely to effect a rather small number of users.

Seem to work for me. Can I get a review of this?
Comment 7 Robert Roth 2012-01-08 23:50:42 UTC
Review of attachment 204835 [details] [review]:

Tested it and works well for me too, and the code also looks fine.
Comment 8 Chris Kühl 2012-01-09 00:18:43 UTC
(In reply to comment #7)
> Review of attachment 204835 [details] [review]:
> 
> Tested it and works well for me too, and the code also looks fine.

Thanks I've pushed this in commit f13ddd3aa0015076961b7764b9c0f7eef79cd3b9.

I'm closing this although there is still the issue that one must use a tool such as dconf-editor to change the colors of CPUs 4-n. I'll open another big for that.
Comment 9 Robert Roth 2012-01-20 06:50:04 UTC
Reopening this, as we've overlooked a small fact, which I've found while investigating bug 668317:
in callbacks.cpp:cb_cpu_color_changed you only iterate over the number of the children of the cpu-colors GVariant, which will allways be 4, as the default value has 4 elements.
Comment 10 Robert Roth 2012-01-20 19:18:42 UTC
Created attachment 205725 [details] [review]
Proposed patch

With this patch added, on startup procman rebuilds the gvariant array, and if it has less color than the number of cores, appends the required number of colors to the gvariant list and saves the new list back into gsettings. Without this extension, this bug is not fixed at all, and system monitor crashes on computers with more cores than 4 (because the colors are stored in the gconf default only for 4 cores), see bug #668317.
Comment 11 Chris Kühl 2012-01-28 01:31:31 UTC
Thanks. See commit 6e966d0cfca266d5f1ea02995b7debc198ff636d.