Created attachment 323845 [details] [review]
trashmonitor: change trash monitoring process

The trash is monitored for state changes - going from empty to non-empty and the
other way round. Monitoring is done by handling change signals from a regular
file monitor. On each signal, an enumeration of the trash contents is started in
order to see if it is empty or not. This causes issues when many files are
trashed, because the gvfs trash backend is flooded with enumeration requests,
resulting in CPU usage spikes. In order to fix this, the "item-count" attribute
of the trash should be queried instead.

Replace asynchronous enumeration with asynchronous information query and update
the trash state based on the "item-count" attribute. Emit state change signal
only when the state actually changes.

(In reply to Razvan Chitu from comment #1)
itor: change trash monitoring process
> 
> The trash is monitored for state changes - going from empty to non-empty and
> the
> other way round. Monitoring is done by handling change signals from a regular
> file monitor. On each signal, an enumeration of the trash contents is
> started in
> order to see if it is empty or not. 

This sounds just wrong. GIO has

G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT

for this purpose.

(In reply to Matthias Clasen from comment #2)
> (In reply to Razvan Chitu from comment #1)
> itor: change trash monitoring process
> > 
> > The trash is monitored for state changes - going from empty to non-empty and
> > the
> > other way round. Monitoring is done by handling change signals from a regular
> > file monitor. On each signal, an enumeration of the trash contents is
> > started in
> > order to see if it is empty or not. 
> 
> This sounds just wrong. GIO has
> 
> G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT
> 
> for this purpose.

Enumeration is done at the moment, but my patch changes that to query info. Should I change the commit message?

(In reply to Razvan Chitu from comment #3)

> Enumeration is done at the moment, but my patch changes that to query info.
> Should I change the commit message?

No, its fine - I guess I just didn't read your commit message carefully enough to pick up on the fact that you did exactly what I am suggesting

Review of attachment 323845 [details] [review]:

fwiw we discussed this with oholy and we agreed it's the way to go.

This is my 2cents to the patch, although Mathias review is the one that counts, comments apply to nautilus patches when appropriate.

::: gtk/gtktrashmonitor.c
@@ +99,3 @@
 {
+  if (monitor->has_trash == has_trash)
+    return;

This will fail in the case below explained. I would use == !!has_trash

@@ +101,3 @@
+    return;
+
+  monitor->has_trash = has_trash;

Don't change this. This is done to avoid setting a boolean out of range. i.e. you can set 7 to the has_trash boolean and it will work, since C doesn't have the concept of boolean. This can be a problem when performing operation like shifting on booleans.

@@ +108,3 @@
+/* Callback used from g_file_query_info_async() - check if the trash has items
+ * by evaluating the "trash-item-count" attribute
+ */

not need to specify in the comment what the callback is from.
Also use G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT in the comment since is the one that helps to search for it in the docs.

Also I would explain in the comment why we use G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT instead of an enumeration. Something along the lines:
/* Use G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT since we only want to know whether the trash is empty or not, not access all the children.
* This is available for the trash backend since it uses a cache. In this way we prevent flooding the trash backend with enumeration request when trashing > 1000 files */

This also applies for nautilus patch.

@@ +145,3 @@
+                           G_FILE_ATTRIBUTE_TRASH_ITEM_COUNT,
+                           G_FILE_QUERY_INFO_NONE,
+                           G_PRIORITY_LOW, NULL,

why low? I would just use the default.

Created attachment 323933 [details] [review]
trashmonitor: change trash monitoring process

The trash is monitored for state changes - going from empty to non-empty and the
other way round. Monitoring is done by handling change signals from a regular
file monitor. On each signal, an enumeration of the trash contents is started in
order to see if it is empty or not. This causes issues when many files are
trashed, because the gvfs trash backend is flooded with enumeration requests,
resulting in CPU usage spikes. In order to fix this, the "item-count" attribute
of the trash should be queried instead.

Replace asynchronous enumeration with asynchronous information query and update
the trash state based on the "item-count" attribute. Emit state change signal
only when the state actually changes.

Review of attachment 323933 [details] [review]:

Looks good to me

I realized that some modifications are needed for the trash backend in order to be sure that valid item-count is reported. See Bug 711459.

Attachment 323933 [details] pushed as 65687ba - trashmonitor: change trash monitoring process

Will this fix be included in 3.20.x too?