! Please note that this is a snapshot of our old Bugzilla server, which is read only since May 29, 2020. Please go to gitlab.xfce.org for our new server !
Refactoring of task-manager.c / fix crash and CPU usage
Status:
RESOLVED: FIXED
Product:
Xfce4-taskmanager
Component:
General

Comments

Description Ivan 83 2018-05-17 02:00:04 CEST
Created attachment 7733 
333

Motivation: original code crash on line:
gtk_tree_model_get (manager->model, iter, XTM_PTV_COLUMN_TIMESTAMP, &old_timestamp, -1);
if multiple processes created/destroyed in short time, for example compilation big app.
Probably this was because iterator from removed_tasks array became invalid.

Changes:
- get_task_list() returns sorted array
- only 1 pass thouth items in manager->model
- bsearch() used for search for pid in sorted arrays
- many cleanups
- src/process-tree-view.c: add check for NULL pointer
- g_debug() -> G_DEBUG_FMT() macro

Results:
- no crashes
- valgrind is happy
- CPU usage: 1,8% -> 0,5%

Probably can fix: https://bugzilla.xfce.org/show_bug.cgi?id=12428
Comment 1 Ivan 83 2018-05-17 04:15:30 CEST
Created attachment 7734 
more floating point corrections, and var sizes
Comment 2 Andre Miranda editbugs 2018-05-26 20:01:58 CEST
Hi Ivan,
Taking a quick look, the patch looks good, though it would be easier to review if you split each change in a separated patch.
Unfortunately I can't apply the patch:

<stdin>:820: trailing whitespace.
		
<stdin>:895: trailing whitespace.
	return (((Task*)a)->pid - ((Task*)b)->pid);	
error: patch failed: src/task-manager-freebsd.c:109
error: src/task-manager-freebsd.c: patch does not apply

Can you please check if you're able to apply it on current master?

Many thanks for taking care of taskmanager, it really needs more love =)
Comment 3 Andre Miranda editbugs 2018-05-26 20:03:31 CEST
Hmmm, probably it depends on changes introduced by the patches from bugs 14401 and 14402.
Going to check them first.
Comment 4 Andre Miranda editbugs 2018-05-26 20:25:01 CEST
That was it.
Unfortunately it introduces a small regression, as soon as taskmanager starts, all processes are green for moment.
Comment 5 Git Bot editbugs 2018-05-26 23:49:12 CEST
rim referenced this bugreport in commit 4d4d0b50736a9b63f49aeadeca51fb3b3691d507

Add a function comparing tasks (bug 14403)

https://git.xfce.org/apps/xfce4-taskmanager/commit?id=4d4d0b50736a9b63f49aeadeca51fb3b3691d507
Comment 6 Git Bot editbugs 2018-05-26 23:53:16 CEST
Landry Breuil referenced this bugreport in commit 159c3c1d0ca1b7397453a08fc46469931af1bb7e

Sort the task list by pid, as we're going to use bsearch() (bug 14403)

https://git.xfce.org/apps/xfce4-taskmanager/commit?id=159c3c1d0ca1b7397453a08fc46469931af1bb7e
Comment 7 Landry Breuil editbugs 2018-05-27 10:32:38 CEST
(In reply to Andre Miranda from comment #4)
> That was it.
> Unfortunately it introduces a small regression, as soon as taskmanager
> starts, all processes are green for moment.

Yup, seeing the same thing when testing.
Comment 8 Landry Breuil editbugs 2018-05-27 10:52:54 CEST
Thinking a bit more, it's "normal" to have them all green, since technically xfce4-taskmanager sees them for the first time - but i agree it's confusing. Will try to make sure they start with the default fg/bg.
Comment 9 Landry Breuil editbugs 2018-05-27 10:58:59 CEST
checking that timestamp is not 0 line 266 fixes that:
if (timestamp != 0 && (timestamp - old_timestamp) <= TIMESTAMP_DELTA)
Comment 10 Git Bot editbugs 2018-05-27 11:07:10 CEST
rim referenced this bugreport in commit f27d270f0c87f7317a16c8afc37df00ec111a03f

Check that treeview isnt NULL in cb_send_signal (bug 14403)

https://git.xfce.org/apps/xfce4-taskmanager/commit?id=f27d270f0c87f7317a16c8afc37df00ec111a03f
Comment 11 Git Bot editbugs 2018-05-27 11:37:27 CEST
Rozhuk Ivan referenced this bugreport in commit fd149b63df44eeace3851ca40812d52684f4e2c6

Rework the way the model is updated (bug 14403)

https://git.xfce.org/apps/xfce4-taskmanager/commit?id=fd149b63df44eeace3851ca40812d52684f4e2c6
Comment 12 Landry Breuil editbugs 2018-05-27 11:37:59 CEST
Testing the model update changes shows no regression, and the logic sounds fine to me - it seems to leak a bit of memory over time but it cant be really worse than before. At least it doesn't crash :)
Now it needs more testing coverage on other platforms, so i've pushed the last bits of your diff to master - please make sure all bits are there...
Comment 13 Ivan 83 2018-05-27 23:12:51 CEST
Created attachment 7745 
speedup app-manager, force types of some vars
Comment 14 Ivan 83 2018-05-28 01:57:02 CEST
Created attachment 7747 
speedup app-manager, force types of some vars
Comment 15 Ali Akcaagac 2018-05-31 09:37:52 CEST
Is it possible to revert both set of patches #14401 and #14403 to the state before having applied them to Git master ?

Back to here:
https://git.xfce.org/apps/xfce4-taskmanager/commit/?id=2e68217ecbd2b8c747829f46dbc1b2297b4b594f

-or-

At least make a new sub-release of xfce4-taskmanager 1.2.x, so there is at least a most recent version that actually compiles and works.

-or-

Have this all put into a sub branch (user banch if you like), so this "specific" version of patches can be tested and fixed.

By committing them all into Git Master without proper verification, you basicly broke xfce4-taskmanager for Linux (and their gazillions of Distributions, different versions of libraries, different versions of core libraries like gtk, glibc and so on).
Comment 16 Landry Breuil editbugs 2018-06-01 13:02:31 CEST
(In reply to Ali Akcaagac from comment #15)

> By committing them all into Git Master without proper verification, you
> basicly broke xfce4-taskmanager for Linux (and their gazillions of
> Distributions, different versions of libraries, different versions of core
> libraries like gtk, glibc and so on).

Sorry, but that not how it works. Distributors should use releases, if they decide to use git master they accept the fact that things might break, and they have to report the failures back, because it's impossible for developers to test all possible environments/os/toolchains/versions.

You're saying that there's been no proper validation, which is not true at all : i've built separately all the commits that were merged, and they compiled and ran fine. I'm working on OpenBSD, Ivan works on FreeBSD. What do you think happens when someone works on Linux, and commits stuff that break the BSDs ? Same thing, we fix it in master.

And on top of this, generally if you don't say what's 'broken' (ie error messages, environment, etc) i can't really help you (but i know you've reported them in https://bugzilla.xfce.org/show_bug.cgi?id=14401#c47, i'm just saying it here for future readers). Asking for a revert is not the way forward....
Comment 17 Ali Akcaagac 2018-06-01 17:20:46 CEST
(In reply to Landry Breuil from comment #16)
> (In reply to Ali Akcaagac from comment #15)
> Sorry, but that not how it works. Distributors should use releases, if they decide to use git master
> they accept the fact that things might break, and they have to report the failures back, because it's
> impossible for developers to test all possible environments/os/toolchains/versions.

Sorry for causing some irritations here. It's not the distro that compiles/offers/distribute Xfce from Git. It was my own compiled Xfce4 from Git master, to help sorting out issues and help improving and testing it. I filled in quite a few bugreports in general within the past years. But I agree! I am seeing a lot of things through my Linux only glasses here, which of course is wrong. It wasn't my intention to complain at all and I apologized if it was perceived as such.

Bug #14403

Reported by:
Ivan 83
Reported on: 2018-05-17
Last modified on: 2018-06-01

People

Assignee:
Mike Massonnet
CC List:
4 users

Version

Attachments

Additional information