1477958 - Lazily load calendar views for performance win

Status: RESOLVED WONTFIX

(Calendar :: Calendar Frontend, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: lazy-load-1.diff

I had a bit of a fight with XUL <deck> today, while trying to finish bug 1477956. 

If the views (or rather the XBL bindings that display the views) are loaded lazily, it solves the problem, and gives us a performance/memory usage win. (Okay, I didn't measure it, but it must, surely.)

Comment 2

[[:MakeMyDay]]

Goeff, does this patch require another one to work?

With this patch the day/week view are not getting initialized properly - they keep empty, while multiweek and month view don't have this issue. If you move a day/week forth and back in day/week view, the items show up again as expected.

Comment 3

[Geoff Lankow (:darktrojan)]

It seems like there's some exceptions thrown that show up everywhere except my computer. Looks like I've got more work to do.

Comment 4

[Geoff Lankow (:darktrojan)]

(In reply to [:MakeMyDay] from comment #2)
> Goeff, does this patch require another one to work?
> 
> With this patch the day/week view are not getting initialized properly -
> they keep empty, while multiweek and month view don't have this issue. If
> you move a day/week forth and back in day/week view, the items show up again
> as expected.

Yeah, that is weird. I can see all-day events in those views, but not others.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: lazy-load-2.diff

Take 2. This time without the problem in comment 2.

Comment 6

[[:MakeMyDay]]

Works like a charme - it probably makes sense to also make the unifinder tree in calendar view (this is the item list that is displayed above the candar view if find event button is active - this is available when customizing the calendar view men) and the unifinder todo tree (for the task view) loading lazy, too.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8994728 [details] [diff] [review]
lazy-load-2.diff

Review of attachment 8994728 [details] [diff] [review]:
-----------------------------------------------------------------

MakeMyDay, since you looked into this already, can you also do the review?

Comment 8

[Geoff Lankow (:darktrojan)]

Hold that thought, this still isn't passing Try.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: lazy-load-3.diff

This would've been a lot earlier if the TC machines and my machine could agree on things.

Comment 10

[[:MakeMyDay]]

Comment on attachment 8995418 [details] [diff] [review]
lazy-load-3.diff

Review of attachment 8995418 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, r+ with comments considered. Since you didn't include lazy loading for the tasks viw and the unifinder as requested in comment 6, please file a follow-up bug for that.

::: calendar/base/content/calendar-views.js
@@ +255,5 @@
>      viewTabs.selectedIndex = getViewDeck().selectedIndex;
>  
> +    if (!("goToDay" in view)) {
> +        view.setAttribute("shown", "true");
> +        await new Promise(resolve => view.addEventListener("bindingattached", resolve, { once: true }));

shouldn't these two lines be flipped?

::: calendar/test/mozmill/cal-recurrence/testBiweeklyRecurrence.js
@@ +101,5 @@
>      controller.assertNode(lookupEventBox("month", EVENT_BOX, 4, 7, null, EVENTPATH));
>  
>      // delete event
>      let box = lookupEventBox("month", EVENT_BOX, 4, 7, null, EVENTPATH);
> +    box.getNode().click();

Can you please leave it to the controller to do the click here and pass the appropriate node to it here and at the other places you changed this?

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to [:MakeMyDay] from comment #10)
> Thanks, r+ with comments considered. Since you didn't include lazy loading
> for the tasks viw and the unifinder as requested in comment 6, please file a
> follow-up bug for that.

Oh, sorry, I got distracted trying to make other things happen in this bug. Since this is blocking bug 1477956, which I want to land ASAP, I've made bug 1479320 for the tasks view and unifinder.

> ::: calendar/base/content/calendar-views.js
> @@ +255,5 @@
> >      viewTabs.selectedIndex = getViewDeck().selectedIndex;
> >  
> > +    if (!("goToDay" in view)) {
> > +        view.setAttribute("shown", "true");
> > +        await new Promise(resolve => view.addEventListener("bindingattached", resolve, { once: true }));
> 
> shouldn't these two lines be flipped?

No, it's adding the attribute that causes the binding to be attached.

> ::: calendar/test/mozmill/cal-recurrence/testBiweeklyRecurrence.js
> @@ +101,5 @@
> >      controller.assertNode(lookupEventBox("month", EVENT_BOX, 4, 7, null, EVENTPATH));
> >  
> >      // delete event
> >      let box = lookupEventBox("month", EVENT_BOX, 4, 7, null, EVENTPATH);
> > +    box.getNode().click();
> 
> Can you please leave it to the controller to do the click here and pass the
> appropriate node to it here and at the other places you changed this?

I know this isn't the right thing to do here, but it's the only way I can get the tests to pass on TaskCluster. They work fine on my own machine but the automation refuses to cooperate.

Comment 12

[[:MakeMyDay]]

(In reply to Geoff Lankow (:darktrojan) from comment #11)
> I know this isn't the right thing to do here, but it's the only way I can
> get the tests to pass on TaskCluster. They work fine on my own machine but
> the automation refuses to cooperate.

What kind of problems did you get with mozmill on taskcluster? If we had issues in the past with tests passing locally but not on automation, this was mostly a timing issue and letting the test pause a ted befor doing a click usually resolved it.

I would have expected that these tests would fail due to bug 1478989, don't they?

Comment 13

[Geoff Lankow (:darktrojan)]

(In reply to [:MakeMyDay] from comment #12)
> What kind of problems did you get with mozmill on taskcluster? If we had
> issues in the past with tests passing locally but not on automation, this
> was mostly a timing issue and letting the test pause a ted befor doing a
> click usually resolved it.

If I use controller.click the click just … doesn't happen. I tried waiting for the box to get the selected attribute. I tried calling sleep(). I tried controller.waitForElement. I tried controller.click with some coordinates. (As you can see I'm a bit frustrated with this.) None of them made the box the active element or enabled the necessary command.

What would help me here is if mozmill could take screenshots of failing situations and upload them. I'm pretty sure mochitest had this ability.

> I would have expected that these tests would fail due to bug 1478989, don't
> they?

No, everything needed to make the tests work was there, just not very usable for humans. :)

Comment 14

[Geoff Lankow (:darktrojan)]

It turns out you CAN make mozmill give you a screenshot, and it does tell me what I'm doing wrong. Now to fix it…

Comment 15

[Geoff Lankow (:darktrojan)]

This isn't really blocking either bug any more. I'm going to push it to the back burner for a while because I'm so frustrated with it.

Comment 16

[[:MakeMyDay]]

Please leave this blocking bug 1478608, because this patch is also the resolution for bug 1476725, which is a fallout from the wx conversion.

Comment 17

[Wayne Mery (:wsmwk)]

(In reply to [:MakeMyDay] from comment #16)

Please leave this blocking bug 1478608, because this patch is also the
resolution for bug 1476725, which is a fallout from the wx conversion.

bug 1476725 has since been resolved - should we remove the dependency?

Is the patch still a valid approach? Is so, perhaps it can be passed on to someone else?

Lastly, can you elaborate on the performance win statement of comment 0 - for example is it significant? dependent on calendar sizes? etc.

Comment 18

[Geoff Lankow (:darktrojan)]

No, this isn't valid any more, and we already have the performance win I was expecting from it. I'm not when or how that happened, but I may have been mistaken in the first place.