822882 - [email] never delay displaying messages or retract messages based on network I/O; always use "refresh" heuristic, do not have bisect case retract

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Currently the e-mail app goes out of its way to try and have the most recent messages in the folder (according to the server) at the top of the UI.  This occurs at the expense of the time-to-display the first messages in the case where the refresh heuristic is not active.  The bad news is that for people where the bisect overflow case occurs because of high message density, it will usually happen allll the time, which sucks.

Per discussion with :gal, our goal is to always display the most recent messages we have for a folder immediately regardless of how many newer messages might exist.

This has various potential UX impacts which is that:

A) We may end up scrolled way down the list of messages.  Because the scrollbar indicator is provided by the platform and we do not artificially inflate the coordinate space (we could! it might be advisable per bug 796474), it might not be super obvious to the user just how far behind they are.  But they can of course scroll up to the top until they run out of messages.  The one glitch is...

B) We don't have an affordance that indicates that synchronization is happening at the top of the message list; just the bottom of the list where we say we are synchronizing and show a throbber.  :gal reallly doesn't like the throbber, but we may want to add a similar UI element at the top of the list that indicates that we are synchronizing more recent messages and so more messages should show up there soon.  (And we could get rid of all the throbbers since we have a progress bar; we probably just want to make 5 the minimum value we ever show for the progress bar rather than 0 so it's obvious that there's a progress bar there.)


I think I can get to this in the next few days, but others should feel free to take.  The file to change is mailslice.js where the terms REFRESH_USABLE_DATA_TIME_THRESH_INBOX, REFRESH_USABLE_DATA_TIME_THRESH_NON_INBOX, or bisect are found.  As long as the constant is just bumped waaaaay up rather than the code eliminated, the unit tests should still pass.  I will take the bug before starting work.

Comment 1

[Chris Lee [:clee]]

We should definitely consider these perf improvements to the Email app for basic usability reasons.  Nom'ing.

Comment 2

[David Scravaglieri [:scravag]]

We would take a patch for this but we will not hold the V1

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Since I just name-checked this bug on the mailing list, the active branch is here:
https://github.com/asutherland/gaia-email-libs-and-more/tree/always-refresh-2

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/127

As far as the (bolstered) IMAP unit tests are concerned, this works.  The ActiveSync tests are no less happier than they have been.  Manual smoke-testing on nightly and unagi seems to be happy, although I want to dogfood a bit and would ideally appreciate testing help from others.

I'm planning to merge in your folder sorting and mutation test unification to trunk and then into this before landing.

I've pushed the changes to my gaia/email-always-refresh branch if anyone wants to test.  One would do something like:
  git remote add asuth git://github.com/asutherland/gaia.git
  git fetch asuth
  git checkout asuth/email-always-refresh


Some fixes that are in here that aren't strictly specific to this patch:

- The bug where a slice would keep synchronizing until it had synchronized the entire folder because the slice was dead and we didn't check for that is addressed.  If you close a slice, it will stop synchronizing in onSyncCompleted no matter what.

- ActiveSync shows a little bit of XHR progress no matter what.  (So does IMAP, but it's less required since IMAP provides ton of updates since it's very granular.)


Some constants that are very important and may want to be tweaked from syncbase are copied below.  Note that in all cases we show the messages we already have before trying to talk to the server.  It's just a question of whether we bother to talk to the server at all because we think we talked to the server recently enough that there's no need to check.

==
/**
 * How recently synchronized does a time range have to be for us to decide that
 * we don't need to refresh the contents of the time range when opening a slice?
 * If the last full synchronization is more than this many milliseconds old, we
 * will trigger a refresh, otherwise we will skip it.
 */
exports.OPEN_REFRESH_THRESH_MS = 10 * 60 * 1000;

/**
 * How recently synchronized does a time range have to be for us to decide that
 * we don't need to refresh the contents of the time range when growing a slice?
 * If the last full synchronization is more than this many milliseconds old, we
 * will trigger a refresh, otherwise we will skip it.
 */
exports.GROW_REFRESH_THRESH_MS = 60 * 60 * 1000;
==

Since we always show what we have and then refresh, all the other complicated/confusing heuristic constants are nuked.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

My testing has shown some duplicated messages on a "futurewards" growth refresh due to the 'startTS' not being quantized to day boundaries in this case.  This is a new edge-case that did not exist previously because we always assumed we were fully synced in that direction because of our sync invariants.  I'll add coverage and fix.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I have addressed the duplicated messages scenario and a related class of problems and provided test coverage.  Additional test coverage for the growing "pastwards" was also added.
https://github.com/asutherland/gaia-email-libs-and-more/commit/4350283f404bbedbd7b126a9dc5e96d1c866a69c

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 715889 [details] [review]
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/127

Vivien, I'm hoping to fix any bit-rot on this and land this after I get the ES5-only patches (bug 844426) merged into master.  I understand for improved coordination during these performance hacking times I'm supposed to get your review and/or sign-off on landings, so here's the heads up on desire to land for this one.  Feel free to e-mail me with more specific guidelines on how you would like to coordinate.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 715889 [details] [review]
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/127

Thanks for coordinating. That sounds good, my main issue is with files dancing around which is not done by this patch :)

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

merged to gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/127

Of course, after doing that, I realized I had introduced new uses of const, so there's also a const fix-up:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/136

and then all of that got merged to gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8417

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

This patch alone hasn't gotten us where we want to go; we still are blocking on network traffic in a lot of cases.  console.log debugging shows that headers are being fetched that should already be known to the system, so this patch may be doing the right thing but it can't do the right thing because of other failures.  My current theory is that we are failing to issue write transactions or they aren't sticking somehow.  Our unit tests are very diligent about checking that we issue writes and that what we write it can also be read out, so it would likely be something emergent.

I'm going to fix the structured logging write-to-disk mechanism (bug 848311) and the mis-directed error cleanup logic issue (bug 826118), then run this to ground.  I'll either reopen this bug or post a link to any follow-up bugs.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

It appears that yesterday's suspicious network traffic was due to internaldate time skew problems.  I'm still investigating that, but the basic situation was that we were experiencing a bisection overflow on our refresh, so our refresh turned into 2 smaller syncs.  These syncs stomped on each other because of internaldate time skew, doing redundant deletions and re-addition of messages.  All of this was otherwise correct in terms of refreshing and not blocking, which is good news.

The new bad news is that this bug did regress bisection-on-initial-sync.  We try a subtract days off of a null date value (our sentinel for 'now') which results in a negative number.  I have a fix for this, but need to provide test coverage.  I'm reopening this bug and tracking the fix here because we would never want the already-landed patch without this fix.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Attachment: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/142

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

initial bisection fix landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/142

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8538

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

There's another problem in the initial bisection which Julien has provided logs for; because of the makeDaysAgo effectively rounding up and our clamping of daysBefore to 1 which does not subtract off when shunting to makeDaysAgo, we end up clamping at 2 days, so we never just give-in and use the 1 days of messages when the density is high enough.

I'm going to fix this by making makeDaysBefore subtract 1 off when converting to makeDaysAgo but will also audit the code-path and add a unit test for this variation too.

Comment 15

[Julien Wajsberg [:julienw]]

the logs are : http://pastie.org/private/dj0eymy0w7cbfsxwjasuw

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

fix for Julien's reported bug landed with tests

landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/146
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/3e2f6db86f68b2332d1f23f0fc1b3f70cf74d89a

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8623
https://github.com/mozilla-b2g/gaia/commit/5e15eb9d741a444dd0368c1f1582baf87f096d82

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

There's a related glitch on the slice growing that's a follow-up to that last patch.  I have an in-progress patch up at:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/148

But I'm a half hour shy of a 16 hour day and there's still an edge-case that generates a similar failure mode.  It needs to be fixed and covered by a test case.  Hoping to get to that tomorrow morning.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Marking fixed again since I landed a fix for the spun-off follow-up bug 851262.  In the interest of simplifying the lives of uplifters, any subsequent fixes will likewise be done on separate bugs.  But I'm optimistic that we've licked the last of the horrible regressions at this point.  Or at least, I added a defensive guard that has us fail in a much less horrible fashion...

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

Will this be included in the wholesale uplift in bug 851124?

Comment 20

[Julien Wajsberg [:julienw]]

Yes John, don't bother uplifting this.

I'll try to find all bugs that are 'automatically' uplifted and mark them as such when I'll close Bug 851124.

Comment 21

[Julien Wajsberg [:julienw]]

landed in Bug 851124

Comment 22

[Julien Wajsberg [:julienw]]

Landed in v1-train as part of Bug 851124 big uplift.