Closed Bug 360479 Opened 18 years ago Closed 18 years ago

Weekview workweek days only takes 50% longer to display

Categories

(Calendar :: Calendar Frontend, defect)

Product:
Calendar
Component:
Calendar Frontend
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
trivial

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: gekacheka, Unassigned)

References

Details

(Keywords: perf)

Attachments

(3 files, 3 obsolete files)

busy week test case: 5 days x 8 hrs/day x 4appts/hr = 160 events
49.11 KB, text/calendar
Details
delete other "removeNonWorkday" methods, not used
3.92 KB, patch
mattwillis
: first-review+
jminta
: second-review+
Details | Diff | Splinter Review
v4: move workweek days calculation to multiday-view so it refreshes just once (v3 nit addressed)
6.45 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061104 Calendar/0.4a1

On a busy workweek schedule, weekview takes ~50% longer to display when "workweek days only" is on than when it is off, even though there are fewer days when it is on.

Reproducible: Always

Steps to Reproduce:
1. view calendar with many events one week, zero events the following week
2. view > workweek days only off
3. time how long it takes to go from zero event week to full week
4. view > workweek days only on
5. time how long it takes to go from zero event week to full week


Actual Results:  
Displaying only workweek days takes ~50% longer
For example, ~6 sec showing all days, ~9 seconds showing workweek days only.

Expected Results:  
Displaying only workweek days should take equal or less time.
Keywords: perf
Version: unspecified → Trunk
(patch -l -p 2 -i file.patch)

Diagnosis:  calendar-decorated-week-view.goToDay was causing calendar-multiday-view.refresh to be called twice when workweek days only are displayed.
  calendar-decorated-week-view.goToDay
    calendar-multiday-view.setDateRange
      calendar-multiday-view.refresh
    calendar-decorated-week-view.removeNonWorkdays
      calendar-multiday-view.setDateList
        calendar-multiday-view-refresh

This fix moves the calculation of workdays into setDateRange, so refresh is only called once.
(multiday-view already has daysOffArray, so this is simpler than the old code: removeNonWorkdays is not needed, doesn't need to recompute days-off from prefs.)

(Future: after this patch, removeNonWorkdays is not used by any view, so calls to it and methods for it may be removed from all views.)
Attachment #245400 - Flags: first-review?(lilmatt)
Also see bug 353325. duplicates?
*** Bug 353325 has been marked as a duplicate of this bug. ***
Comment on attachment 245400 [details] [diff] [review]
move workweek days calculation to multiday-view so it refreshes just once

>--- mozilla/calendar/base/content/calendar-multiday-view.xml	2006-11-05 23:52:58.000000000 -0500
>         <parameter name="aStartDate"/>
>         <parameter name="aEndDate"/>
>         <body><![CDATA[
>-          //dump ("setDateRange\n");
>-          this.mDateList = null;
>-
>-          this.mStartDate = aStartDate.getInTimezone(this.mTimezone);
>-          this.mStartDate.isDate = true;
>-          this.mStartDate.makeImmutable();
>-
>-          this.mEndDate = aEndDate.getInTimezone(this.mTimezone);
>-          this.mEndDate.isDate = true;
>-          this.mEndDate.makeImmutable();
>-
>-          // this function needs to be smarter, and needs to compare
>-          // the current date range and add/remove, instead of just
>-          // replacing.
>-
>-          this.refresh();
>+          aStartDate = aStartDate.getInTimezone(this.mTimezone);
>+          aStartDate.isDate = true;
>+          aEndDate = aEndDate.getInTimezone(this.mTimezone);
>+          aEndDate.isDate = true;
As a general rule, aVariables should be left in their pristine, as-passed-in state, so that other functions can use them as well without having to be concerned if they've been modified. Please .clone() them to another variable if you need to modify them.

>+          if (this.mDisplayDaysOff) { 
Trailing space on this line

>+            aStartDate.makeImmutable();
>+            aEndDate.makeImmutable();
>+            this.mDateList = null;
>+            this.mStartDate = aStartDate;
>+            this.mEndDate = aEndDate;
>+            // this function needs to be smarter, and needs to compare
>+            // the current date range and add/remove, instead of just
>+            // replacing.
Could we add this intelligence in this patch? If not, what's holding us back, and should we file and reference a spin-off to finish it?

>+            function isDayOff(queryDay) {
>+              function isQueryDay(dayOff) { return queryDay == dayOff; }
>+              return weekdaysOffArray.some(isQueryDay);
>+            }
These vars should be named aQueryDay and aDayOff

>+            var dateList = new Array();
>+            for (var d = aStartDate.clone(); d.compare(aEndDate) <= 0;) {
>+                if (!isDayOff(d.weekday)) { 
Trailing space on this line


Regarding your comment about removing all calls to removeNonWorkdays(), shouldn't that be in this patch as well?

r-, but this looks promising. Most of this is just style.
Attachment #245400 - Flags: first-review?(lilmatt) → first-review-
(patch -l -p 2 -i file.patch) 

Nits addressed, comments added.  

Responding to the review:

1. The calDateTime objects passed into the method were not modified, so callers could have reused the parameters.  But as requested, bound to new variables instead.

2. The variables bound to the arguments were rebound to the normalized form (normalized to the display timezone), so the unnormalized version would no longer be available.  Adding a variant name to the namespace to keep the unnormalized datetime around may lead to further errors, since someone could make a change that uses the unnormalized datetime where the normalized version should be used.  So added code to assign the unnormalized variables to null.

3. The "this function needs to be smarter" comment refers to the "refresh" method call.  That comment has been there since version 1.1, so from the name of the file, it is probably meant for a view that doesn't yet exist, e.g., a 3 day view that advances by one day at a time, so there is significant overlap between the old view and the next and refresh could reuse existing box layout.  The only case currently where there is any overlap in the dates is when switching weekview between all-weekdays and workdays-only, a relatively rare operation which doesn't need to be optimized at this time.  

(Other removeNonWeekdays methods deleted in separate patch forthcoming)
Attachment #245400 - Attachment is obsolete: true
Attachment #245536 - Flags: first-review?(lilmatt)
(patch -l -p 2 -i file.patch)
Attachment #245537 - Flags: first-review?(lilmatt)
Comment on attachment 245536 [details] [diff] [review]
v2: move workweek days calculation to multiday-view so it refreshes just once (nits addressed)

>--- mozilla/calendar/base/content/calendar-multiday-view.xml	2006-11-05 23:52:58.000000000 -0500
>+          if (this.mDisplayDaysOff) {
>+            startDate.makeImmutable();
>+            endDate.makeImmutable();
>+            this.mDateList = null;
>+            this.mStartDate = startDate;
>+            this.mEndDate = endDate;
>+            // For a true multiday view (e.g, 3 days advanced by one day
Please add a blank line before this comment ^^^ for readability

>+            const weekdaysOffArray = this.daysOffArray;
Constants should either start with a k, be all caps, or both. Must this be a constant?

>+            function isDayOff(aQueryDay) {
>+              function isQueryDay(aDayOff) { return aQueryDay == aDayOff; }
Please put the "return" on a separate line.

>+              return weekdaysOffArray.some(isQueryDay);
>+            }
Please add some comments to this area to clear up why we have these functions nested this way. 

>+            var dateList = new Array();
>+            for (var d = startDate.clone(); d.compare(endDate) <= 0;) {
>+                if (!isDayOff(d.weekday)) {
>+                    var workday = d.clone();
>+                    workday.makeImmutable();
>+                    dateList.push(workday);
>+                }
>+                d.day += 1;
>+                d.normalize();
>+            }
This for loop suddently switches to 4 space indenting. Please use 2 space to be consistent.

>+            this.setDateList(dateList.length, dateList);
>+          }
>         ]]></body>
>       </method>
> 

This is all just style or comment nits, so r=lilmatt with those fixed.
Attachment #245536 - Flags: first-review?(lilmatt) → first-review+
Comment on attachment 245537 [details] [diff] [review]
delete other "removeNonWorkday" methods, not used

r=lilmatt
Attachment #245537 - Flags: first-review?(lilmatt) → first-review+
(patch -l -p 2 -i file.patch)

See comment #2 for description.
Attachment #245536 - Attachment is obsolete: true
Attachment #247273 - Flags: second-review?(jminta)
Attachment #245537 - Flags: second-review?(jminta)
Comment on attachment 247273 [details] [diff] [review]
v3: move workweek days calculation to multiday-view so it refreshes just once (v2 nits addressed)

+          // make sure unnormalized version not used below
+          aStartDate = aEndDate = null; 
This seems like unnecessary protection.  The comments above indicate which variables should be used.

+            // Use array.some for array.contains
I don't understand this comment at all.

+            function isDayOff(aQueryDay) {
+              return kWeekdaysOffArray.some(function isQueryDay(aDayOff) {
+                                              return aQueryDay == aDayOff;
+                                            });
+            }
Why not just if (this.mDaysOffArray.indexOf(d.weekday)  != -1)
That'd be cleaner, and I think the performance difference would be very close to 0, since you have to declare fewer variables.
Attachment #247273 - Flags: second-review?(jminta) → second-review+
Attachment #245537 - Flags: second-review?(jminta) → second-review+
(patch -l -p 2 -i file.patch)

Good catch --- I forgot indexOf was added to JavaScript at the same time as some.  This patch fixes that and also converts a previous use of some to indexOf for marking weekend attributes.  

(I disagree that comments are enough of a deterrent against using the unnormalized dates, since the point of the convention of not modifying 'aXxx' parameter bindings [required by first-review] seems to be that people shouldn't have to read all the code between the top of the method and the use in order to use the parameter, so they won't read the comments in between either.  Also, such in-between code is often omitted from patches, so reviewers may easily miss it.  So I left nulling the unnormalized parameters in this patch.)
Attachment #247273 - Attachment is obsolete: true
patch checked in
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Thanks!  Please also check in attachment 245537 [details] [diff] [review].
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #14)
Checked in attachment 245537 [details] [diff] [review] on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070106 Calendar/0.4a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: