Open Bug 1477958 Opened Last year Updated Last month

Lazily load calendar views for performance win

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: darktrojan, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

Attached patch lazy-load-1.diff (obsolete) — Splinter Review
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.)
Attachment #8994459 - Flags: review?(philipp)
Duplicate of this bug: 1476725
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.
It seems like there's some exceptions thrown that show up everywhere except my computer. Looks like I've got more work to do.
(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.
Attached patch lazy-load-2.diff (obsolete) — Splinter Review
Take 2. This time without the problem in comment 2.
Attachment #8994459 - Attachment is obsolete: true
Attachment #8994459 - Flags: review?(philipp)
Attachment #8994728 - Flags: review?(philipp)
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 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?
Attachment #8994728 - Flags: review?(philipp) → review?(makemyday)
Hold that thought, this still isn't passing Try.
Blocks: 1478608
Attached patch lazy-load-3.diffSplinter Review
This would've been a lot earlier if the TC machines and my machine could agree on things.
Attachment #8994728 - Attachment is obsolete: true
Attachment #8994728 - Flags: review?(makemyday)
Attachment #8995418 - Flags: review?(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?
Attachment #8995418 - Flags: review?(makemyday) → review+
(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.
Blocks: 1477956
(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?
(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. :)
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…
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.
No longer blocks: 1477956, 1478608
Please leave this blocking bug 1478608 this this patch is also the resolution for bug 1476725, which is a fallout from the wx conversion.
Blocks: 1478608
Assignee: geoff → nobody
Status: ASSIGNED → NEW
Keywords: perf
You need to log in before you can comment on or make changes to this bug.