Closed Bug 315960 Opened 19 years ago Closed 18 years ago

profile view-switching code

Categories

(Calendar :: Calendar Frontend, defect)

Product:
Calendar
Component:
Calendar Frontend
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dmosedale, Unassigned)

Details

(Keywords: perf)

Attachments

(4 files, 5 obsolete files)

checked in:multiday-batch v2
4.06 KB, patch
dmosedale
: first-review+
Details | Diff | Splinter Review
checked in: add a compare method to calIDuration; v2
2.21 KB, patch
dmosedale
: first-review+
Details | Diff | Splinter Review
checked in: reuse month grid
10.70 KB, patch
Details | Diff | Splinter Review
multiday view fruit
11.64 KB, patch
dmosedale
: first-review+
Details | Diff | Splinter Review 
This is also very slow.  We should pick off any low-hanging fruit here before Lightning 0.1 also.
Attached patch multiday-batch (obsolete) — — Splinter Review 
2109               dayEventsBox.setStartEndMinutes(this.mStartMin, this.mEndMin);
2110               dayEventsBox.setAttribute("orient", orient);
2111               dayEventsBox.date = d;
2112               dayEventsBox.calendarView = this;

Each of these calls forces the dayEventsBox to call its own relayout method, which is a clear waste of effort.  Then,
2151           this.onResize();
forces them to all do it over again.

This patch implements startBatchChange and endBatchChange methods for the columns, to eliminate these extra calls.

Average refresh times:
    Prior to patch: 370ms
    After patch: 130ms :-)
Attachment #202604 - Flags: first-review?(dmose)
Attached patch reuse month grid (obsolete) — — Splinter Review 
There was a nice suggestion in the code that we should reuse the grid-boxes and just re-number them.  This patch does that, along with correcting for cases where the currently shown month involves a different number of weeks than the new month to be shown.

Average refresh times:
   Prior to patch: 130ms
   After patch: 45ms when both old month and new month have the same number of weeks, 60ms going to fewer weeks, 80ms going to more weeks
Attachment #202617 - Flags: first-review?(dmose)
Comment on attachment 202604 [details] [diff] [review]
multiday-batch

This looks good.  The one thing I would suggest would be to use a counter instead of a boolean (eg mOpenBatchCount).  This avoids a problem in the case where two different callers open overlapping batches.  With a boolean, the relayout calls resume as soon as the first batch is done, rather than once both batches are done.
Attachment #202604 - Flags: first-review?(dmose) → first-review-
Comment on attachment 202617 [details] [diff] [review]
reuse month grid

>+          this.mItemData = new Array();
>+          while(this.dayitems.firstChild) {

I suspect that using .hasChildNodes() here might be a bit faster, even.

>+                this.dayitems.removeChild(this.dayitems.lastChild);
>+          }

It might be worth finding out how the DOM child list is implemented.  If its
a linked list with no explicit lastChild pointer, removing .firstChild would make this loop O(n) instead of O(n^2) w.r.t. the length of the list.  

>@@ -657,31 +664,22 @@
>       </method>
> 
>       <method name="relayout">
> 	<body><![CDATA[
>           function createXULElement(el) {
>               return document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", el);
>           }
> 
>-	  var gridrows = this.monthgridrows;
>-
>-          // XXX - reuse the grid boxes!
>-          // clear out the grid
>           if (this.mSelectedItem)
>-            this.mSelectedItem = null;
>-
>+            this.selectedItem = null;

What's the reason for this change?

> 
> 	  // we're going to cheat and make sure that the first full
> 	  // month is "even".  The only time when this wouldn't be
> 	  // valid is if the first of the month actually falls to
> 	  // whatever the first weekday is that we're displaying.

So I figured out why "even" and "odd" were chosen here.  The idea is that months alternate colors (well, classes, really) to make them visually distinct from one another.  If one imagines a view where there are days from four different months in the view, those names make at least some sense.  Any opinion on whether we should keep these names, or move to something more obvious, like, say, "currentMonth" (given that we dont currently have any views that can contain days from four months)?

> 	  var isEven = 0;
> 	  if (this.mStartDate.day == 1)
>@@ -689,16 +687,107 @@
> 
>           var dateBoxes = [];
> 
>           var first = true;
>           var lastMonth = this.mStartDate.month;
>           var lastWeekNo = null;
>           var curRow = null;
> 
>+	  if (!this.mStartDate || !this.mEndDate)
>+            return;

Shouldnt the above throw an exception rather than quietly returning?

The rest of the patch needs significantly more high-level comments, very briefly explaining what each clause is doing and why.  If you can re-submit with that, it'll make the rest of the review much easier.

Thanks!
Attachment #202617 - Flags: first-review?(dmose) → first-review-
Changes mInBatchChange to mLayoutBatchCount.  I included 'Layout' in the name to avoid confusion with the batch-counting done by the calICompositeObserver.
Attachment #202604 - Attachment is obsolete: true
Attachment #203474 - Flags: first-review?(dmose)
Comment on attachment 203474 [details] [diff] [review]
checked in:multiday-batch v2

r=dmose
Attachment #203474 - Flags: first-review?(dmose) → first-review+
Attachment #203474 - Attachment description: multiday-batch v2 → checked in:multiday-batch v2
Attached patch reuse month grid v2 (obsolete) — — Splinter Review 
A lot less code duplication in this version, and a lot more comments.  Hopefully, everything that's going on here should be pretty clear.
Attachment #202617 - Attachment is obsolete: true
Attachment #203885 - Flags: first-review?(dmose)
Comment on attachment 203885 [details] [diff] [review]
reuse month grid v2

This is definitely looking better.

* please use hasChildNodes in while() loop tests, as this is more semantically correctly and probably faster

* I'll whip up a patch that adds a compare method to calIDuration 

* The relayout method is now large enough that it's becoming hard to read.  I would like to propose a slightly different factoring with two functions that get called by relayout: createNewGrid() and reuseExistingGrid().  In addition to cutting down on function size, this has the advantage that the code flow is easier to read because it's not filled with |if (!canReuse)| cases.  


* Another possible optimization might be to, when a row is removed because it's not necessary for this particular redraw, keep it around for the next time a redraw occurs that needs one more row.
Attachment #203885 - Flags: first-review?(dmose) → first-review-

        Attachment #204604 -
        Flags: first-review?(mvl)

    
    

    
  
Comment on attachment 204604 [details] [diff] [review]
add a compare method to calIDuration, v1

>Index: calendar/base/src/calDuration.cpp
>+    if ( thisInSeconds < otherInSeconds ) {
>+      *aResult = -1;
>+    } else if ( thisInSeconds < otherInSeconds ) {

I guess you want that to be thisInSeconds > otherInSeconds.

r=mvl with that fixed.

        Attachment #204604 -
        Flags: first-review?(mvl) → first-review+

    
    

    
  


  
mvl: whoops!  good catch; here's the updated patch.

        Attachment #204604 -
        Attachment is obsolete: true

        Attachment #204972 -
        Flags: first-review+

    
    

    
  
Comment on attachment 204972 [details] [diff] [review]
checked in: add a compare method to calIDuration; v2

Patch checked in.

        Attachment #204972 -
        Attachment description: add a compare method to calIDuration; v2 → checked in: add a compare method to calIDuration; v2

    
    

    
  

      
      
      
      

        Attached patch
          
          
        reuse month grid v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Splits out the relayout method into two separate methods.  Also, since I was rewriting most of this relayout stuff anyway, I took the liberty of killing all the confusing even/odd/first/lastMonth stuff in favor of some easier to follow checks/variables.

I'm not in favor of just keeping around a row.  The simplest way to do that would be to collapse it, but having monthgridrows not accurately reflect what's displayed (because it also contains the collapsed row) scares me.  Actually pulling the row out of the DOM, but keeping it in memory also seems like it might be asking for trouble.  Either way, there's a lot of fruit hanging lower than that, specifically in addItem/onGetItems.

        Attachment #203885 -
        Attachment is obsolete: true

        Attachment #205308 -
        Flags: first-review?(dmose)

    
    

    
  
Comment on attachment 205308 [details] [diff] [review]
reuse month grid v3

Looks great; r=dmose with comments addressed.

>Index: mozilla/calendar/base/content/calendar-month-view.xml
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/content/calendar-month-view.xml,v
>retrieving revision 1.8
>diff -p -U8 -r1.8 calendar-month-view.xml
>--- mozilla/calendar/base/content/calendar-month-view.xml	22 Nov 2005 01:49:20 -0000	1.8
>+++ mozilla/calendar/base/content/calendar-month-view.xml	8 Dec 2005 18:10:50 -0000
>@@ -193,16 +193,22 @@
>
>+          // Remove all the old events
>+          this.mItemData = new Array();
>+          while(this.dayitems.firstChild) {
>+                this.dayitems.removeChild(this.dayitems.lastChild);
>+          }

This wants to use hasChildNodes, I think.

>         ]]></body>
>       </method>
> 
>       <method name="addItem">
>         <parameter name="aItem"/>
>         <body><![CDATA[
>           for each (ed in this.mItemData) {
>             if (ed.item == aItem ||
>@@ -545,16 +551,19 @@
>         <getter><![CDATA[
>           if (this.mSelectedDayBox)
>             return this.mSelectedDayBox.date.clone();
>         ]]></getter>
>         <setter><![CDATA[
>           if (this.mSelectedDayBox)
>             this.mSelectedDayBox.box.selected = false;
> 
>+          if (!val)
>+              return;
>+

Doesn't this want to throw instead of returning, since calling this without a valid parameter seems like it represents a bug in the calling code?

>@@ -661,95 +670,212 @@
>       </method>
> 
>       <method name="relayout">
> 	<body><![CDATA[
>           function createXULElement(el) {
>               return document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", el);
>           }
> 
>-	  var gridrows = this.monthgridrows;
>+          if (this.mSelectedItem) {
>+            this.selectedItem = null;
>+          }
>+          if (this.mSelectedDayBox) {
>+            this.selectedDay = null;
>+          }
> 
>-          // XXX - reuse the grid boxes!
>-          // clear out the grid
>-          if (this.mSelectedItem)
>-            this.mSelectedItem = null;
>+          if (!this.mStartDate || !this.mEndDate)
>+            return;

Seems like it might make sense to have this throw instead of returning also, since calling without those things having been set seems like an error.

>-          if (this.mSelectedDayBox)
>-            this.mSelectedDayBox = null;
>+          // If we've already drawn a view once, then in almost all cases we
>+          // can resuse most of the grid.  We may need to add or subtract a row
>+          // but this is still much faster than recreating all rows.
>+          var canReuse = false;
>+          if (this.mDateBoxes) {
>+            canReuse = true;
>+            var oldDuration = this.mDateBoxes[this.mDateBoxes.length-1].date.subtractDate(this.mDateBoxes[0].date);
>+            var newDuration = this.mEndDate.subtractDate(this.mStartDate);
>+            newDuration.isNegative = true;
>+            newDuration.normalize();
>+            newDuration.addDuration(oldDuration);
>+
>+            switch (newDuration.days + newDuration.weeks*7) {

Note that there is now a calIDuration.compare method which you could use here, should you so desire.  It'd simplify the code, ever so slightly.

        Attachment #205308 -
        Flags: first-review?(dmose) → first-review+

    
    

    
  


  
Patch checked in.  Includes all the review comments except for using .compare on the durations, because it turns out I need to know the difference (for when the new views land), not just that one is bigger.

        Attachment #205308 -
        Attachment is obsolete: true

    
    

    
  
I switched from my debug-build-with-gcc setup to using the branch nightlies with Thunderbird 1.5 on my slowest machine (which is a 1.5 Ghz Win XP box), and things aren't exactly snappy, but they're well within reason for a 0.1 release.  Following Simon's advice and removing this from the Lightning 0.1 blocker list.
No longer blocks: lightning-0.1

    
    

    
  


  
I took a shot at all the low-hanging fruit I could find in the multiday-view.  Mostly what I was concentrating on was avoiding repetitive relayout calls, and avoiding excess DOM manipulation.  Accordingly, this patch includes 3 bits:
-Reuse existing columns whenever possible
-Fixes to avoid additional relayout of columns when setting attributes like item-context
-Place addEvent's relayout in a timeout, to allow onGetResult to return faster and minimize repetitive work here.

I can switch in around 110ms for an empty view now, so we're getting close to a point where getItems speed is starting to hold us up, but that's a good thing!

        Attachment #219153 -
        Flags: first-review?(dmose)

    
    

    
  
Comment on attachment 219153 [details] [diff] [review]
multiday view fruit

In general, this looks great; Im really looking forward to the performance wins!  r=dmose with the various changes mentioned.

I would expect mEventMapTimer to be an nsITimer.  How about using the name mEventMapTimeoutID?

>@@ -465,17 +469,17 @@
>         <body><![CDATA[
>           var needsrelayout = false;
>           if (aAttr == "orient") {
>               if (this.getAttribute("orient") != aVal)
>                   needsrelayout = true;
>           }
> 
>           if (aAttr == "context" || aAttr == "item-context")
>-              needsrelayout = true;
>+              needsrelayout = false;

I really wish I knew why this clause was thought to be necessary in the first place.  That said, I dont see why relayout should be required.
For readability, though, how about turning the entire quoted fragment above into a single switch() statement?

>       <method name="clear">
>         <body><![CDATA[
>-          while (this.bgbox.lastChild)
>+          while (this.bgbox && this.bgbox.lastChild)
>               this.bgbox.removeChild(this.bgbox.lastChild);
>-          while (this.topbox.lastChild)
>+          while (this.topbox && this.topbox.lastChild)

How about using hasChildren() in the while statements here to avoid potential unnecessary list traversal and convey intent more clearly?

The relayout changes still end up with the function being fairly long, and requiring a decent bit of reading to get an overview.  To improve this, could you factor out the code used for the initial creation of the various boxes into separate functions (e.g. create{DavEvents,DayHeader,Label}Box)
and put them at the top of this method, along with removeKidBoxes?

>+              } else {
>+                  labelbox = createXULElement("box");
>+                  labelbox.setAttribute("flex", "1");
>+                  labelbox.setAttribute("class", "calendar-day-label-box");
>+
>+                  var label = createXULElement("label");

Would it be likely to win an interesting amount of time to re-use both the label elements along with the labelbox?

        Attachment #219153 -
        Flags: first-review?(dmose) → first-review+

    
    

    
  
(In reply to comment #18)
> Would it be likely to win an interesting amount of time to re-use both the
> label elements along with the labelbox?
> 
Right now we re-use both or neither, but since it's extremely unlikely to have one and not the other, we don't look for that case.

"multiday view fruit" checked in.

Bug 340025 filed as a follow-up to sort out some of the "context" and "item-context" issues.


    
    

    
  
I'm going to call this bug fixed, since we've had so many checkins on it already.  Please file additional bugs for further performance wins.
Status: NEW → RESOLVED
Closed: 18 years ago
Component: Lightning Only → Calendar Views
Resolution: --- → FIXED
QA Contact: lightning → views

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: