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 720298 - Resource usage improvements to the calendar
Resource usage improvements to the calendar
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-12-12 01:39 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-12-16 17:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
calendar: Don't ever force reload (5.54 KB, patch)
2013-12-12 01:43 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
calendar: Don't rebuild the entire calendar widget when choosing a date (5.21 KB, patch)
2013-12-12 01:43 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
calendar: Don't rebuild the entire calendar widget when choosing a date (5.21 KB, patch)
2013-12-13 17:08 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2013-12-12 01:39:04 UTC
This was spotted by Adam Williamson and others in the "Memory Leak" bug.

While not necessarily a leak (the GC wasn't being triggered), let's not
do any more than we need to, and not rebuild the entire calendar layout
every time we open the calendar or set the date.
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-12-12 01:43:22 UTC
Created attachment 264034 [details] [review]
calendar: Don't ever force reload
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-12-12 01:43:26 UTC
Created attachment 264035 [details] [review]
calendar: Don't rebuild the entire calendar widget when choosing a date

It's inefficient and wasteful. Combined with the JS GC not being great,
it leaks around 100 actors waiting to be GC'd. Yikes.
Comment 3 Giovanni Campagna 2013-12-13 16:52:58 UTC
Review of attachment 264034 [details] [review]:

So how about event backends that require a pull from the upstream service? Do we expect e-d-s to handle it?
Comment 4 Giovanni Campagna 2013-12-13 17:03:38 UTC
Review of attachment 264035 [details] [review]:

::: js/ui/calendar.js
@@ +669,3 @@
+            this._monthLabel.text = this._selectedDate.toLocaleFormat(this._headerFormat);
+
+        if (!this._calendarBegin || _sameMonth(this._selectedDate, this._calendarBegin))

Reversed logic?
As I read the code, you want to rebuild if the new selected date is in a different month.
Comment 5 Jasper St. Pierre (not reading bugmail) 2013-12-13 17:08:33 UTC
Created attachment 264160 [details] [review]
calendar: Don't rebuild the entire calendar widget when choosing a date

It's inefficient and wasteful. Combined with the JS GC not being great,
it leaks around 100 actors waiting to be GC'd. Yikes.



Good catch.
Comment 6 Jasper St. Pierre (not reading bugmail) 2013-12-16 17:45:03 UTC
Attachment 264034 [details] pushed as 9fce12d - calendar: Don't ever force reload
Attachment 264160 [details] pushed as cc4659f - calendar: Don't rebuild the entire calendar widget when choosing a date


I asked Matthew Barnes about this, and he said we should be fine.