951806 - upgrading contacts DB to 1.3 takes too long

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Contacts, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

At each new version of b2g we need to upgrade the contacts DB to account for differences in the schema.  The upgrade to v1.3 seems to take an excessive amount of time.  I recently tried to install reference-workload-x-heavy (2000 contacts) on 1.3 and it took over 30 minutes.  (I killed it after 30 minutes...)

In my opinion this is slow enough to be a blocker for 1.3 since users will think contacts is just completely broken.

Comment 1

[Julien Wajsberg [:julienw]]

I have good idea on how to optimize this but this is really risky so we need to do it soon and have a lot of QA testing the migration.

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+. agreeing on description

Comment 3

[Mike Lee [:mlee]]

David,
Assigning this to Julien since he seems to have a plan. Please reassign if someone else is more appropriate.

Comment 4

[Julien Wajsberg [:julienw]]

Hey Ben,

my plate is quite full right now, can you help here?

The first thing I'd like to do is take measures of each migration steps, and see if there is one that is especially long.

From there, 2 possibilities:
* either one is really long, and we need to focus on that one
* either they're about equally long, and we need to try to do the loop only once, eg: in update steps, push the function that will change the cursor to an array, and loop once at the end and running all functions in the array.

The first possibility is less risky that the second one.

So Ben, if you can already start the first step, taking measures, it would immensely help.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Unfortunately I am working a CS v1.3 blocker with a short deadline, so I don't have any time at the moment.  Can you have David talk to Mike about getting this on our schedule for a future sprint?

Or do Reuben or Gregor have any bandwidth?

Comment 6

[Jon Hylands [:jhylands]]

If you install the x-large reference workload onto a 1.2 phone, and let the database update, you'll have a large 1.2 database. At that point, you could manually install that database into a 1.3 phone, and get an isolated 1.2 -> 1.3 contacts upgrade to profile.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Taking this for the next sprint.  I will probably do this next week in the second half of the week.

Comment 8

[Mike Lee [:mlee]]

Jon, switching this to you for pickup next sprint 2014.02.03-14.

Comment 9

[Reuben Morais [:reuben]]

Yoink.

Comment 10

[Peter Dolanjski [:pdol]]

What is the user experience when this upgrade is happening?  Does this happen after the update install such that the contacts app becomes unresponsive?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Peter Dolanjski [:pdol] from comment #10)
> What is the user experience when this upgrade is happening?  Does this
> happen after the update install such that the contacts app becomes
> unresponsive?

You get the contacts title bar and alphabet jump bar on the right.  The rest of the app is a white screen until the upgrade is complete.  See bug 932893 about possibly adding a spinner during the upgrade.

Either way, anything over a few minutes is way too long IMO.

Comment 12

[Julien Wajsberg [:julienw]]

Measurements for an upgrade on a Peak, 1.2 -> 1.3:

01-31 11:48:19.289  2603  2603 I Gecko   : -*- ContactDB component: upgrade schema from: 14 to 18 called!
01-31 11:48:19.289  2603  2603 I Gecko   : -*- ContactDB component: Upgrade step: 14
01-31 11:48:19.289  2603  2603 I Gecko   : -*- ContactDB component: Fix array properties saved as scalars
01-31 11:48:39.199  2603  2603 I Gecko   : -*- ContactDB component: Upgrade step: 15
01-31 11:48:39.199  2603  2603 I Gecko   : -*- ContactDB component: Fix Date properties
01-31 11:48:54.489  2603  2603 I Gecko   : -*- ContactDB component: Upgrade step: 16
01-31 11:48:54.489  2603  2603 I Gecko   : -*- ContactDB component: Fix array with null values
01-31 11:49:11.689  2603  2603 I Gecko   : -*- ContactDB component: Upgrade step: 17
01-31 11:49:11.689  2603  2603 I Gecko   : -*- ContactDB component: Adding the name index
01-31 11:49:30.979  2603  2603 I Gecko   : -*- ContactDB component: Upgrade finished

so that's about 15 seconds for each step.

I'll measure now 1.1 -> 1.2 (which was really fast), and 1.1 -> 1.3. Maybe we have a regression in IndexedDB iteration?

Comment 13

[Julien Wajsberg [:julienw]]

1.1 -> 1.3:

01-31 11:53:32.249  2832  2832 I Gecko   : -*- ContactDB component: upgrade schema from: 12 to 18 called!
01-31 11:53:32.249  2832  2832 I Gecko   : -*- ContactDB component: Upgrade step: 12
01-31 11:53:32.249  2832  2832 I Gecko   : -*- ContactDB component: Add phone substring to the search index if appropriate for country
01-31 11:53:32.249  2832  2832 I Gecko   : -*- ContactDB component: Upgrade step: 13
01-31 11:53:32.249  2832  2832 I Gecko   : -*- ContactDB component: Cleaning up empty substring entries in telMatch index
01-31 11:53:46.339  2832  2832 I Gecko   : -*- ContactDB component: Upgrade step: 14
01-31 11:53:46.339  2832  2832 I Gecko   : -*- ContactDB component: Fix array properties saved as scalars
01-31 11:54:04.999  2832  2832 I Gecko   : -*- ContactDB component: Upgrade step: 15
01-31 11:54:04.999  2832  2832 I Gecko   : -*- ContactDB component: Fix Date properties
01-31 11:54:21.139  2832  2832 I Gecko   : -*- ContactDB component: Upgrade step: 16
01-31 11:54:21.139  2832  2832 I Gecko   : -*- ContactDB component: Fix array with null values
01-31 11:54:40.889  2832  2832 I Gecko   : -*- ContactDB component: Upgrade step: 17
01-31 11:54:40.889  2832  2832 I Gecko   : -*- ContactDB component: Adding the name index
01-31 11:55:04.949  2832  2832 I Gecko   : -*- ContactDB component: Upgrade finished

So it's about the same thing, except step 12 which was not executed with substringMatching off.

I'm gonna try the solution in comment 4, point 2 now.

Note that I also saw an issue with the last upgrading path, I'll fix in the same time.

Comment 14

[Julien Wajsberg [:julienw]]

1.1 -> 1.3 on a Buri:

01-31 12:05:43.289  2772  2772 I Gecko   : -*- ContactDB component: upgrade schema from: 12 to 18 called!
01-31 12:05:43.289  2772  2772 I Gecko   : -*- ContactDB component: Upgrade step: 12
01-31 12:05:43.289  2772  2772 I Gecko   : -*- ContactDB component: Add phone substring to the search index if appropriate for country
01-31 12:05:43.289  2772  2772 I Gecko   : -*- ContactDB component: Upgrade step: 13
01-31 12:05:43.289  2772  2772 I Gecko   : -*- ContactDB component: Cleaning up empty substring entries in telMatch index
01-31 12:07:25.759  2772  2772 I Gecko   : -*- ContactDB component: Upgrade step: 14
01-31 12:07:25.759  2772  2772 I Gecko   : -*- ContactDB component: Fix array properties saved as scalars
01-31 12:10:57.569  2772  2772 I Gecko   : -*- ContactDB component: Upgrade step: 15
01-31 12:10:57.569  2772  2772 I Gecko   : -*- ContactDB component: Fix Date properties
01-31 12:14:16.659  2772  2772 I Gecko   : -*- ContactDB component: Upgrade step: 16
01-31 12:14:16.659  2772  2772 I Gecko   : -*- ContactDB component: Fix array with null values
01-31 12:17:27.989  2772  2772 I Gecko   : -*- ContactDB component: Upgrade step: 17
01-31 12:17:27.989  2772  2772 I Gecko   : -*- ContactDB component: Adding the name index
01-31 12:20:48.100  2772  2772 I Gecko   : -*- ContactDB component: Upgrade finished

12->13: irrelevant
13->14: ~1min40sec
14->15: ~3min30sec
15->16: ~3min45sec
16->17: ~3min10sec
17->18: ~3min20sec

So there are all similar, except the first one. I don't have a reason for this but now I'll try my solution and see whether this looks better.

Comment 15

[Julien Wajsberg [:julienw]]

I have a working patch that makes the 1.1 -> 1.3 upgrade on Peak last 35 seconds (from 1min30sec).

Now I'm testing it on my own 1.1 data as it seems to have all sort of strange cases that made our upgrade code break in the past. If that passes, then I'm really confident that everything will.

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Try is https://tbpl.mozilla.org/?tree=Try&rev=cc6d78661608

I unfortunately can't compare on Buri, because my Buri died since then.

Comment 17

[Julien Wajsberg [:julienw]]

I could make the Buri work again, so here is the result on the Buri: down to 2min30s from 15min30sec.

Now, I see something else happening: while looping, the delay between the first and second value is about 20ms, but the delay between the lsat one and the one before is 100ms!

Will try to see if something happens memorywise too.

Comment 18

[Julien Wajsberg [:julienw]]

Note that all these measures were done on master.

Comment 19

[Julien Wajsberg [:julienw]]

new try: https://tbpl.mozilla.org/?tree=Try&rev=82bd8dde0136
(I haven't enabled the correct config)

Comment 20

[Julien Wajsberg [:julienw]]

new try: https://tbpl.mozilla.org/?tree=Try&rev=56f6dc0ba48d
this time with the Gaia tests, I hope.

Comment 21

[Gregor Wagner [:gwagner]]

This should not be a 1.3 blocker. It's a nice to have but it's not blocking us from shipping a phone imho.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Gregor Wagner [:gwagner] from comment #21)
> This should not be a 1.3 blocker. It's a nice to have but it's not blocking
> us from shipping a phone imho.

Is this because we don't expect v1.1 phones to be updated to v1.3?  Blocking a user from user their contacts for 30+ minutes (my experience) without any user feedback about what is happening seems really bad to me.

Comment 23

[Gregor Wagner [:gwagner]]

(In reply to Ben Kelly [:bkelly] from comment #22)
> (In reply to Gregor Wagner [:gwagner] from comment #21)
> > This should not be a 1.3 blocker. It's a nice to have but it's not blocking
> > us from shipping a phone imho.
> 
> Is this because we don't expect v1.1 phones to be updated to v1.3?  Blocking
> a user from user their contacts for 30+ minutes (my experience) without any
> user feedback about what is happening seems really bad to me.

Changing this code that late in the game is high risk. We don't have good testing for this code part and if this breaks the user ends up with a bricked phone. We had multiple errors in the upgrade path before so I am super paranoid about it.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Gregor Wagner [:gwagner] from comment #23)
> Changing this code that late in the game is high risk. We don't have good
> testing for this code part and if this breaks the user ends up with a
> bricked phone. We had multiple errors in the upgrade path before so I am
> super paranoid about it.

Hmm, ok. However, I think "high risk" is a bit different than saying "nice-to-have".  It seems we need to do something here, even if its add a spinner to the contacts app so they know what the phone is trying to do.

Comment 25

[Jason Smith [:jsmith]]

(In reply to Ben Kelly [:bkelly] from comment #22)
> (In reply to Gregor Wagner [:gwagner] from comment #21)
> > This should not be a 1.3 blocker. It's a nice to have but it's not blocking
> > us from shipping a phone imho.
> 
> Is this because we don't expect v1.1 phones to be updated to v1.3?  Blocking
> a user from user their contacts for 30+ minutes (my experience) without any
> user feedback about what is happening seems really bad to me.

FWIW - this is a normal expected partner use case actually. Partners aren't picking up 1.2, so the expected data migration path users will experience is to upgrade from 1.1 to 1.3, given that our production devices are all running 1.1 right now.

Comment 26

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #23)
> (In reply to Ben Kelly [:bkelly] from comment #22)
> > (In reply to Gregor Wagner [:gwagner] from comment #21)
> > > This should not be a 1.3 blocker. It's a nice to have but it's not blocking
> > > us from shipping a phone imho.
> > 
> > Is this because we don't expect v1.1 phones to be updated to v1.3?  Blocking
> > a user from user their contacts for 30+ minutes (my experience) without any
> > user feedback about what is happening seems really bad to me.
> 
> Changing this code that late in the game is high risk. We don't have good
> testing for this code part and if this breaks the user ends up with a
> bricked phone. We had multiple errors in the upgrade path before so I am
> super paranoid about it.

The problem in this situation is that data migration is deemed as a critical use case that must work flawlessly between versions. The risk in not fixing this bug is that users will likely think that the contacts app can't load up the old contacts from 1.1, as 30 minutes is far too long for a user to patiently wait. I don't think a UX improvement with a spinner is going to help too much here, since I don't think a user would wait 30 minutes for an upgrade even if the UX was clear that was how long it was going to take.

I understand the concern about patch risk, but I don't think removing this as a blocker is the right path here. What we should be doing instead is trying to control patch complexity + having QA assist in regression testing the patch here. When Hubert gets back from CNY, I can ping him to help out here if needed.

Curious - what's the projected upgrade time for other reference workloads from 1.1 --> 1.3? How does this compare to 1.01 --> 1.1?

Comment 27

[Julien Wajsberg [:julienw]]

I can add some data points:

* I tested this patch using the very same data set (basically: my own contacts) that triggered bugs in the past. Therefore I'm very confident that it works at least as good as the current code, except for the substring matching part.

* The patch fixes one real upgrade bug affecting contacts that were imported from SIM (basically, upgraded contacts-from-SIM could not be searched from the SMS app except the first one. This is not a regression because this didn't work in previous versions anyway). This bug could be fixed in another simpler patch if necessary. And this makes me think that I should make the upgrade path fix use a new upgrade step, so I'll need to modify this patch.

* I did my tests using the 1.1 -> 1.3 upgrade path, especially because it was easier for me. But most of the used time (I'd say, roughly 5/6e) happens in the 1.2 -> 1.3 upgrade path, so please don't focus too much on the 1.1 -> 1.3 upgrade path.

* The Gaia Unit Tests and Integration Tests are passing (except intermittents) in the try build. Not sure this is relevant though, as I don't know which tests we have here.

Some answers now:

> I don't think a user would wait 30 minutes for an upgrade even if the UX was clear that was
> how long it was going to take.

Note that even if the user kills the contacts app, the upgrade still happens in background. If the user reboots or shuts down the phone, then it stops and effectively aborts the update, which will start from scratch th enext time. So that _might_ be acceptable, even if I rather think like Ben here.

> How does this compare to 1.01 --> 1.1?

We can't really compare because we don't have workloads for 1.0.1. Back in the time we didn't pay much attention to this because we assumed users in 1.0.1 didn't have lots of contacts.

But in my tests 1.1 -> 1.2 is less than 2 minutes on Buri with the 2k-contacts workload. (and I get 15min for 1.1 -> 1.3, I don't find Ben's 30 min with my tests, but maybe this measurement can change if the phone does other things in background).

Therefore I think this should block (I didn't think so in the past but once you see it you understand), and that we'll absolutely need to test this with real-world data from both countries using substring matching and others.

Hope this can help making an informed decision.

Comment 28

[Julien Wajsberg [:julienw]]

Attachment: patch v2

New try: https://tbpl.mozilla.org/?tree=Try&rev=3712e7dec6e9

I won't be able to test this new patch before 24h or so.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

The other annoying UX issue is that because gaia doesn't know the upgrade is going on it doesn't set a wake lock.  So my device keeps sleeping during the upgrade which probably extends it quite a bit.

Just noting that here.  I haven't had a chance to try the new patch yet.

Comment 30

[Julien Wajsberg [:julienw]]

Ah yeah, good point, I think I didn't let my device sleep while I was testing.

I'll test patch v2 soon with both data sets (my own contacts + the 2k-contact workload) and report here.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

I suppose the contact manager could enable the cpu wake lock from the parent process while the upgrade is running, couldn't it?  Does that interface exist?

Comment 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8369270 [details] [diff] [review]
patch v2

This patch has issues.

Comment 33

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Tests I did with both workloads:
* first upgrade to current central, then upgrade to a version with my patch
* upgrade directly from 1.1 to a central including my patch
* did a search from the SMS app

I can't push a new try because the tree is currently closed, but this looks good.

Comment 34

[Julien Wajsberg [:julienw]]

(In reply to Ben Kelly [:bkelly] from comment #31)
> I suppose the contact manager could enable the cpu wake lock from the parent
> process while the upgrade is running, couldn't it?  Does that interface
> exist?

I don't know but I guess it's possible, yep. Could you file another bug for this?

Comment 35

[Reuben Morais [:reuben]]

(In reply to Ben Kelly [:bkelly] from comment #31)
> I suppose the contact manager could enable the cpu wake lock from the parent
> process while the upgrade is running, couldn't it?  Does that interface
> exist?

That's a good idea, I filed bug 967119.

Comment 36

[Julien Wajsberg [:julienw]]

New try is https://tbpl.mozilla.org/?tree=Try&rev=21fc2f648b54

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8369544 [details] [diff] [review]
patch v3

Upgrading reference-workload-heavy now takes 1 minute or less.  Very nice!

Comment 38

[Reuben Morais [:reuben]]

Comment on attachment 8369544 [details] [diff] [review]
patch v3

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

Why did you only use the code for versions 13 and higher? It makes more sense (for code maintenance and to reason about the upgrade logic) if we use this new setup for all versions, that way the upgrade will consistently run all schema changes, and then all data changes.

This is awesome! Looks really good, but I'd like to see a new patch that uses this logic for all versions that loop over the data.

::: dom/contacts/fallback/ContactDB.jsm
@@ +186,5 @@
> +
> +    function scheduleValueUpgrade(upgradeFunc) {
> +      var length = valueUpgradeSteps.push(upgradeFunc);
> +      if (DEBUG) {
> +        debug('Scheduled a value upgrade function, index ' + (length - 1));

nit: if (DEBUG) debug…

@@ +190,5 @@
> +        debug('Scheduled a value upgrade function, index ' + (length - 1));
> +      }
> +    }
> +
> +    debug("upgrade schema from: " + aOldVersion + " to " + aNewVersion + " called!");

if (DEBUG)

@@ +515,4 @@
>                        }
>                      }
> +                  }
> +                }, this

nit: JS callback hell makes it hard to find out what this is (heh), can you add /*thisArg*/ before the |this|? That, or keep the |.bind(this)|.

@@ +541,5 @@
>            }
>  
> +          let modified = removeEmptyStrings(value.search.parsedTel);
> +          let modified2 = removeEmptyStrings(value.search.tel);
> +          return (modified || modified2);

This is great :D

@@ +679,3 @@
>        },
>        function upgrade17to18() {
> +        // this upgrade function has been moved to the next upgrade path

Please link to a bug comment explaining why this change was made, since it's not directly related to the bug in the checkin message. Alternatively, you could move this fix to a separate bug.

@@ +699,2 @@
>  
> +        scheduleValueUpgrade(function upgradeValue17to18(value) {

This should be called |upgradeValue18to19|, right?

@@ +717,5 @@
>  
>      let index = aOldVersion;
>      let outer = this;
> +
> +    /* This function run all upgrade functions that are in the valueUpgradeSteps

s/run/runs/

@@ +723,5 @@
> +     * - they are synchronous
> +     * - they take the value as parameter and modify it directly. They must not
> +     *   create a new object.
> +     * - they return a boolean true/false; true if the value was actually
> +     *   changed

nit: "they must have the following" instead of "they have", "they must be" instead of "they are", etc.

@@ +734,5 @@
> +      objectStore.openCursor().onsuccess = function(event) {
> +        let cursor = event.target.result;
> +        if (cursor) {
> +          let changed = false;
> +          var value = cursor.value;

nit: I was going to ask why |var| instead of |let| here, but then I realized this file is not consistent at all about it. Actually, just pass cursor.value in the two lines below.

@@ +751,5 @@
> +      };
> +    }
> +
> +    function finish() {
> +      debug("Upgrade finished");

if (DEBUG)

Comment 39

[Mike Lee [:mlee]]

1.3+ per comment 25, comment 26, comment 27, and comment 29.

Comment 40

[Reuben Morais [:reuben]]

I agree with gwagner's risk assessment here. If we *really* want to land this on 1.3 then bug 932803 has to land first, or together, so we have better coverage.

Comment 41

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #38)
> Comment on attachment 8369544 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8369544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why did you only use the code for versions 13 and higher? It makes more
> sense (for code maintenance and to reason about the upgrade logic) if we use
> this new setup for all versions, that way the upgrade will consistently run
> all schema changes, and then all data changes.

Well, I figured this was useless work because nobody would use this code these days... And we don't have workloads to test this on, so this also makes it dangerous to touch this code as we wouldn't be really able to test it, and would take a lot more time to test it manually.

> 
> This is awesome! Looks really good, but I'd like to see a new patch that
> uses this logic for all versions that loop over the data.

I can still do that.

> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +186,5 @@
> > +
> > +    function scheduleValueUpgrade(upgradeFunc) {
> > +      var length = valueUpgradeSteps.push(upgradeFunc);
> > +      if (DEBUG) {
> > +        debug('Scheduled a value upgrade function, index ' + (length - 1));
> 
> nit: if (DEBUG) debug…

I wanted to start always having braces ;) But I can change do this ;)


> @@ +190,5 @@
> > +        debug('Scheduled a value upgrade function, index ' + (length - 1));
> > +      }
> > +    }
> > +
> > +    debug("upgrade schema from: " + aOldVersion + " to " + aNewVersion + " called!");
> 
> if (DEBUG)

I actually left over this one on purpose: it happens rarely and I think we want to see it (along with the "upgrade finished" log) even in non-DEBUG mode. In the past, we had problems when we forgot calling "next" and having these logs always show up would help not forgetting this. I'm not talking about displaying _all_ upgrade debug logs, but only the first and last ones.

What do you think ?

> 
> @@ +515,4 @@
> >                        }
> >                      }
> > +                  }
> > +                }, this
> 
> nit: JS callback hell makes it hard to find out what this is (heh), can you
> add /*thisArg*/ before the |this|? That, or keep the |.bind(this)|.

I'll come over with something better, yep.

> 
> @@ +679,3 @@
> >        },
> >        function upgrade17to18() {
> > +        // this upgrade function has been moved to the next upgrade path
> 
> Please link to a bug comment explaining why this change was made, since it's
> not directly related to the bug in the checkin message. Alternatively, you
> could move this fix to a separate bug.

Or at least a separate patch/commit, yep.

> @@ +751,5 @@
> > +      };
> > +    }
> > +
> > +    function finish() {
> > +      debug("Upgrade finished");
> 
> if (DEBUG)

see above.

Comment 42

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #40)
> I agree with gwagner's risk assessment here. If we *really* want to land
> this on 1.3 then bug 932803 has to land first, or together, so we have
> better coverage.

While I agree that bug 932803 should land, I don't think this would lower the risk so much. What could have lowered the risk is to have lots of existing tests about migration issues, but there is no chance we can write something comprehensive out of nothing.

Bug 932803 is more about getting basic migration tests working, so that we can add tests in future bugs, but I think it's unfortunately too late for this to be effective now :(

Just my 2 cents.

Comment 43

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #42)
> (In reply to Reuben Morais [:reuben] from comment #40)
> > I agree with gwagner's risk assessment here. If we *really* want to land
> > this on 1.3 then bug 932803 has to land first, or together, so we have
> > better coverage.
> 
> While I agree that bug 932803 should land, I don't think this would lower
> the risk so much. What could have lowered the risk is to have lots of
> existing tests about migration issues, but there is no chance we can write
> something comprehensive out of nothing.
> 
> Bug 932803 is more about getting basic migration tests working, so that we
> can add tests in future bugs, but I think it's unfortunately too late for
> this to be effective now :(

You're right, this would still be risky even with that bug, but basic tests is better than no tests. That bug at least gives us coverage of the most common case, upgrading from all versions to DB_VERSION.

Comment 44

[Reuben Morais [:reuben]]

Comment on attachment 8369544 [details] [diff] [review]
patch v3

(In reply to Julien Wajsberg [:julienw] from comment #41)
> > Why did you only use the code for versions 13 and higher? It makes more
> > sense (for code maintenance and to reason about the upgrade logic) if we use
> > this new setup for all versions, that way the upgrade will consistently run
> > all schema changes, and then all data changes.
> 
> Well, I figured this was useless work because nobody would use this code
> these days... And we don't have workloads to test this on, so this also
> makes it dangerous to touch this code as we wouldn't be really able to test
> it, and would take a lot more time to test it manually.

You're right, after bug 889239 that code won't even run for the case I had in mind, developers flashing builds.

> > @@ +190,5 @@
> > > +        debug('Scheduled a value upgrade function, index ' + (length - 1));
> > > +      }
> > > +    }
> > > +
> > > +    debug("upgrade schema from: " + aOldVersion + " to " + aNewVersion + " called!");
> > 
> > if (DEBUG)
> 
> I actually left over this one on purpose: it happens rarely and I think we
> want to see it (along with the "upgrade finished" log) even in non-DEBUG
> mode. In the past, we had problems when we forgot calling "next" and having
> these logs always show up would help not forgetting this. I'm not talking
> about displaying _all_ upgrade debug logs, but only the first and last ones.
> 
> What do you think ?

Ok. Ideally we'd only output those when something actually goes wrong, eg. "finish() was never called!!!!!11one", but the helpful/spammy ratio on this one is good :)

r=me with nits addressed. Thanks for fixing this!

Oh, and remember to describe the fix, not the bug, in the commit message. Something like "Merge openCursor calls during a contacts DB upgrade".

Comment 45

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #43)
> (In reply to Julien Wajsberg [:julienw] from comment #42)
> > (In reply to Reuben Morais [:reuben] from comment #40)
> > > I agree with gwagner's risk assessment here. If we *really* want to land
> > > this on 1.3 then bug 932803 has to land first, or together, so we have
> > > better coverage.
> > 
> > While I agree that bug 932803 should land, I don't think this would lower
> > the risk so much. What could have lowered the risk is to have lots of
> > existing tests about migration issues, but there is no chance we can write
> > something comprehensive out of nothing.
> > 
> > Bug 932803 is more about getting basic migration tests working, so that we
> > can add tests in future bugs, but I think it's unfortunately too late for
> > this to be effective now :(
> 
> You're right, this would still be risky even with that bug, but basic tests
> is better than no tests. That bug at least gives us coverage of the most
> common case, upgrading from all versions to DB_VERSION.

oki, will work on that bug now.

Comment 46

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #38)
> @@ +515,4 @@
> >                        }
> >                      }
> > +                  }
> > +                }, this
> 
> nit: JS callback hell makes it hard to find out what this is (heh), can you
> add /*thisArg*/ before the |this|? That, or keep the |.bind(this)|.

I actually kept the |bind(this)| call, because the point of this bug is not to change this stuff.
 
> @@ +734,5 @@
> > +      objectStore.openCursor().onsuccess = function(event) {
> > +        let cursor = event.target.result;
> > +        if (cursor) {
> > +          let changed = false;
> > +          var value = cursor.value;
> 
> nit: I was going to ask why |var| instead of |let| here, but then I realized
> this file is not consistent at all about it. Actually, just pass
> cursor.value in the two lines below.

I used "let" and kept it like this in the end, because I find the code easier to read and less cluttered with this additional variable.

Comment 47

[Julien Wajsberg [:julienw]]

Attachment: 1. fix name index upgrade (v1)

Splitting this issue in its own commit.

Keeping r=reuben

Comment 48

[Julien Wajsberg [:julienw]]

Attachment: 2. Merge value upgrade steps in one cursor loop

new try is https://tbpl.mozilla.org/?tree=Try&rev=2426023f06cf

Comment 49

[Julien Wajsberg [:julienw]]

Comment on attachment 8369996 [details] [diff] [review]
2. Merge value upgrade steps in one cursor loop

Fixed nits, keeping r=reuben

new try is https://tbpl.mozilla.org/?tree=Try&rev=2426023f06cf

Will try again this new patch on a device before asking for check in.

Comment 50

[Julien Wajsberg [:julienw]]

https://tbpl.mozilla.org/?tree=Try&rev=ad2c7a205056 is a new rebased try (with bug 932803 inside), hoping it will look better (?).

Comment 51

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 52

[Julien Wajsberg [:julienw]]

The bug is ready to land but we need bug 932803 before this. So estimations is to land tomorrow.

Comment 53

[Julien Wajsberg [:julienw]]

Comment on attachment 8369996 [details] [diff] [review]
2. Merge value upgrade steps in one cursor loop

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

Per Gregor's request, some more reviews on this

Comment 54

[Julien Wajsberg [:julienw]]

BTW, I was wrong for some comments made earlier: there was actually no upgrade between 1.1 and 1.2, so the reference workloads we have for 1.1 are perfectly suitable to test 1.2 -> 1.3. Maybe my own contacts are from an earlier db though.

Comment 55

[Julien Wajsberg [:julienw]]

New try containing the new patch for bug 932803: https://tbpl.mozilla.org/?tree=Try&rev=333b68f671c1

Comment 56

[Jon Hylands [:jhylands]]

Julien - if you install a reference workload in a 1.2 phone, and then save the database, you have a 1.2 reference workload database, which you can then manually install on a 1.3 device to measure the performance hit on the upgrade.

Comment 57

[Gregor Wagner [:gwagner]]

Comment on attachment 8369996 [details] [diff] [review]
2. Merge value upgrade steps in one cursor loop

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +741,5 @@
> +          });
> +
> +          if (changed) {
> +            cursor.update(value);
> +          }

Lets check that the contacts are the same if !changed in DEBUG only.

Comment 58

[Julien Wajsberg [:julienw]]

(In reply to Jon Hylands [:jhylands] from comment #56)
> Julien - if you install a reference workload in a 1.2 phone, and then save
> the database, you have a 1.2 reference workload database, which you can then
> manually install on a 1.3 device to measure the performance hit on the
> upgrade.

Yep Jon, that's what I did originally, but then I saw in the code the db version was he same, hence my previous comment.

Comment 59

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8369996 [details] [diff] [review]
2. Merge value upgrade steps in one cursor loop

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

This looks good to me, though it's obviously tricky code and I will feel much better about it once it's been tested thoroughly. Let's talk about the second comment:

::: dom/contacts/fallback/ContactDB.jsm
@@ +188,5 @@
> +      var length = valueUpgradeSteps.push(upgradeFunc);
> +      if (DEBUG) debug('Scheduled a value upgrade function, index ' + (length - 1));
> +    }
> +
> +    debug("upgrade schema from: " + aOldVersion + " to " + aNewVersion + " called!");

This should go back into a |if (DEBUG)| block.

@@ +734,5 @@
> +        let cursor = event.target.result;
> +        if (cursor) {
> +          let changed = false;
> +          let value = cursor.value;
> +          valueUpgradeSteps.forEach(function(upgradeFunc, i) {

I'm concerned that running this tight loop could keep us away from the event loop for a significant amount of time (enough to cause jank maybe?). What kind of time does a full iteration take on your reference workloads?

@@ +735,5 @@
> +        if (cursor) {
> +          let changed = false;
> +          let value = cursor.value;
> +          valueUpgradeSteps.forEach(function(upgradeFunc, i) {
> +            if (DEBUG) debug("Running upgrade function " + i);

Silly, but |i| is unchanged here so your debug output isn't all that useful.

@@ +750,5 @@
> +      };
> +    }
> +
> +    function finish() {
> +      debug("Upgrade finished");

|if (DEBUG)|

Comment 60

[Julien Wajsberg [:julienw]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #59)
> Comment on attachment 8369996 [details] [diff] [review]
> 2. Merge value upgrade steps in one cursor loop
> 
> Review of attachment 8369996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, though it's obviously tricky code and I will feel
> much better about it once it's been tested thoroughly. Let's talk about the
> second comment:
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +188,5 @@
> > +      var length = valueUpgradeSteps.push(upgradeFunc);
> > +      if (DEBUG) debug('Scheduled a value upgrade function, index ' + (length - 1));
> > +    }
> > +
> > +    debug("upgrade schema from: " + aOldVersion + " to " + aNewVersion + " called!");
> 
> This should go back into a |if (DEBUG)| block.

This was actually done on purpose. I think this is very helpful to always see this along with the "upgrade finished" line. In the past, we had bad and stupid bugs that prevented the code to finish, and therefore having these 2 lines always showing up would help seeing something went wrong (this is quite related to what we were discussing a few fays ago, about ensuring an upgrade can go to its end).

More over the noise ratio is very low with these lines, since it would be displayed only once per migration.

> 
> @@ +734,5 @@
> > +        let cursor = event.target.result;
> > +        if (cursor) {
> > +          let changed = false;
> > +          let value = cursor.value;
> > +          valueUpgradeSteps.forEach(function(upgradeFunc, i) {
> 
> I'm concerned that running this tight loop could keep us away from the event
> loop for a significant amount of time (enough to cause jank maybe?). What
> kind of time does a full iteration take on your reference workloads?

The full iteration takes ~2,5 min on the big workload on Buri, but each cursor takes between 20 and 110ms (although I don't know if that's the IndexedDB loading time or these functions execution time).

I can add logs to measure this and report.

> 
> @@ +735,5 @@
> > +        if (cursor) {
> > +          let changed = false;
> > +          let value = cursor.value;
> > +          valueUpgradeSteps.forEach(function(upgradeFunc, i) {
> > +            if (DEBUG) debug("Running upgrade function " + i);
> 
> Silly, but |i| is unchanged here so your debug output isn't all that useful.

Well, it actually is? "forEach" functions are getting passed 3 args: the value, the index, and the array itself. Moreover, I've seen it work ;)

> 
> @@ +750,5 @@
> > +      };
> > +    }
> > +
> > +    function finish() {
> > +      debug("Upgrade finished");
> 
> |if (DEBUG)|

See above.

Comment 61

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Julien Wajsberg [:julienw] from comment #60)
> > This should go back into a |if (DEBUG)| block.
> This was actually done on purpose.

Ok, that makes sense to me. 

> The full iteration takes ~2,5 min on the big workload on Buri, but each
> cursor takes between 20 and 110ms (although I don't know if that's the
> IndexedDB loading time or these functions execution time).

Right, the time I'm concerned about is the amount of time where we're actually running JS, not when we're waiting for IDB. But if it's a max of 110ms I don't think it matters. Are there certain contacts that could make it take longer maybe? Something that our reference workload doesn't include?

> Well, it actually is? "forEach" functions are getting passed 3 args: the
> value, the index, and the array itself. Moreover, I've seen it work ;)

OMG JS is magical and confusing. Ok!

Comment 62

[Julien Wajsberg [:julienw]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #61)
> 
> > The full iteration takes ~2,5 min on the big workload on Buri, but each
> > cursor takes between 20 and 110ms (although I don't know if that's the
> > IndexedDB loading time or these functions execution time).
> 
> Right, the time I'm concerned about is the amount of time where we're
> actually running JS, not when we're waiting for IDB. But if it's a max of
> 110ms I don't think it matters. Are there certain contacts that could make
> it take longer maybe? Something that our reference workload doesn't include?

Yes probably, I'll try to measure this as well. But I don't think the variance would be that big. Anyway, will try to measure all of this.

> OMG JS is magical and confusing. Ok!

You meant "awesome", right ? :)

Comment 63

[Julien Wajsberg [:julienw]]

I basically finished the patch with the fixed comments, but I want to test properly the new code, and it has some more logs so that I can do the measures, so I won't upload it now.

Will definitely be finished Monday, and then I'll wait for another feedback from Gregor and Bent after giving the measurements from my Buri.

Comment 64

[Julien Wajsberg [:julienw]]

Here are my measurements on a Buri:

* we use ~4sec to run all first-pass upgrade steps
* we use 3-4ms to run all the value-specific upgrade steps (12 -> 19 without substring matching) for the first contact.
* we use ~40ms to load the second contact

Now, here is what's interesting:
* we consistently use 3-4ms when the screen is enabled, but 8-9ms when the screen is disabled. This is a good case for bug 967119 :)
* we use ~35-50ms to load the next contact when the screen is enabled, ~70ms to load the next contact when the screen is disabled.

I guess this lifts Ben's concerns (I won't NI him because he's in PTO).

Now I'm cleaning up the final patch!

Comment 65

[Julien Wajsberg [:julienw]]

Attachment: 2. Merge value upgrade steps in one cursor loop v2

carrying over r=reuben,gwagner,bent

New try is: https://tbpl.mozilla.org/?tree=Try&rev=bab0b966330f

Comment 66

[Julien Wajsberg [:julienw]]

Attachment: 2. Merge value upgrade steps in one cursor loop v3

Oops sorry, I left some unused functions in my previous patch.

Here is a new one, with a new try:
https://tbpl.mozilla.org/?tree=Try&rev=94a6a285d25c

Comment 67

[Julien Wajsberg [:julienw]]

Comment on attachment 8373429 [details] [diff] [review]
2. Merge value upgrade steps in one cursor loop v3

Gregor, wants to have a final look on this?

Comment 68

[Julien Wajsberg [:julienw]]

try has issues, will sort this out tomorrow.

Comment 69

[Julien Wajsberg [:julienw]]

Attachment: 2. Merge value upgrade steps in one cursor loop v4

carrying over r=reuben,bent,gregor

I cleaned up a little too much in my previous patch...

Comment 70

[Julien Wajsberg [:julienw]]

Yet another new try: https://tbpl.mozilla.org/?tree=Try&rev=1df1455d4209

Comment 71

[Julien Wajsberg [:julienw]]

This is ready to go in !

Comment 72

[Julien Wajsberg [:julienw]]

This is a try for v1.3: https://tbpl.mozilla.org/?tree=Try&rev=db3359a56871

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/cd1fa983ce08
https://hg.mozilla.org/integration/b2g-inbound/rev/c3d50a1b6006

Comment 74

[Julien Wajsberg [:julienw]]

QA: When this one will land on 1.3, we'll need migration testing from real device with real-world data, and see whether the contacts app still loads after ~3 minutes.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd1fa983ce08
https://hg.mozilla.org/mozilla-central/rev/c3d50a1b6006

Comment 76

[Tony Chung [:tchung]]

(In reply to Julien Wajsberg [:julienw] from comment #74)
> QA: When this one will land on 1.3, we'll need migration testing from real
> device with real-world data, and see whether the contacts app still loads
> after ~3 minutes.

Thanks.  Massimo's team at Telefonica has volunteered to help with testing here.   Massimo, please request any more information you need here to help with testing tomorrow's 1.3 build.    Thanks!

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/309bcbb7e134
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/40a612e68c16

Comment 78

[Loli (:lolimartinezcr)]

We need a "build that updated via OTA", for this reason is necesary to know about what device we do We need buid updated via OTA, for this reason is necesary to know about WHAT DEVICE and what version is updated (perhaps 1.1 to 1.3??)

Comment 79

[Ryan VanderMeulen [:RyanVM]]

Why are you asking me? I'm not the assignee of this bug...

Comment 80

[Julien Wajsberg [:julienw]]

Loli: please test 1.1 to 1.3 and 1.2 to 1.3.

Both with some meaningful and real world data.

Thanks !

Comment 81

[Tony Chung [:tchung]]

(In reply to Julien Wajsberg [:julienw] from comment #80)
> Loli: please test 1.1 to 1.3 and 1.2 to 1.3.
> 
> Both with some meaningful and real world data.
> 
> Thanks !

Hubert, can you take a look at this bug and see if you can reproduce?  if so, please include some metrics around contacts app if its causing perf regressions.  If you need help with how to measure perf numbers, please reach out to mike lee or mason chang.   You can start by confirming if there is a contacts db regression on the latest build from 1.1 -> 1.3 and 1.2 -> 1.3.   Thanks!

Comment 82

[Julien Wajsberg [:julienw]]

We don't want to measure perf regressions (this bug actually _improved_ the performance), we want to ensure the correctness of the fix. I mean, check that the user can still access his contacts after an upgrade, with a good and meaningful contact DB.

Comment 84

[Hubert Lu[:hlu] <[e-mail]>]

Buri fails to upgrade to v1.3,please see bug#935059 comment 89, so I verify this in Inari and below is the result.

*Result*
- V1.1 to V1.3 (5m51sec)
  02-22 07:10:30.842 I/Gecko   ( 1650): -*- ContactDB component: upgrade schema from: 12 to 19 called!
  02-22 07:10:31.213 I/Adreno200-EGLSUB( 1650): <CreateImage:896>: Android Image
  02-22 07:10:31.213 I/Adreno200-EGLSUB( 1650): <GetImageAttributes:1161>: RGBX_8888
  02-22 07:10:33.105 E/Profiler( 1650): BPUnw: [1 total]thread_unregister_for_profiling(me=0x1b955b0)  (NOT REGISTERED) 
  02-22 07:10:33.135 E/Profiler( 1650): BPUnw: [1 total]thread_unregister_for_profiling(me=0x1b95350)  (NOT REGISTERED) 
  02-22 07:10:33.695 E/GeckoConsole( 1891): Content JS INFO at app://communications.gaiamobile.org/contacts/js/fb/datastore_migrator.js:287 in onidle: Idle event!!!. FB Migration about to start
  02-22 07:10:33.845 E/GeckoConsole( 1891): Content JS WARN at app://communications.gaiamobile.org/contacts/js/fb/datastore_migrator.js:206 in openIndexedDB/req.onsuccess: FB Migration: The database does not exist
  02-22 07:16:21.184 I/Gecko   ( 1650): -*- ContactDB component: Upgrade finished

- V1.2 to V1.3 (6m53sec)
  02-21 04:13:16.525 I/Gecko   (  525): -*- ContactDB component: upgrade schema from: 14 to 19 called!
  02-21 04:13:16.946 I/Adreno200-EGLSUB(  525): <CreateImage:896>: Android Image
  02-21 04:13:16.946 I/Adreno200-EGLSUB(  525): <GetImageAttributes:1161>: RGBX_8888
  02-21 04:13:19.488 E/GeckoConsole(  747): Content JS INFO at app://communications.gaiamobile.org  /contacts/js/fb/datastore_migrator.js:287 in onidle: Idle event!!!. FB Migration about to start
  02-21 04:13:19.679 E/GeckoConsole(  747): Content JS WARN at app://communications.gaiamobile.org/contacts/js/fb/datastore_migrator.js:206 in 
  02-21 04:20:09.218 I/Gecko   (  525): -*- ContactDB component: Upgrade finished


*ENV*
 - OS ver.: v1.1
   - Gaia: 53e2a70d85fb3748d0768218a5efffe5806073f0
   - Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/c630289d6388
   - BuildID: 20131009041203

 - OS ver.: v1.2
   - Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
   - Gecko: https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/c2b030d87d12
   - BuildID: 20140222004002

 - OS ver.: v1.3
   - Gaia: 5084b832f3b536f60ccdb38c14fd6162e5bfbac0
   - Gecko: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5f3959008ee8
   - BuildID: 20140222004002

Comment 85

[Reuben Morais [:reuben]]

I think you wanted to post that in a different bug?

Comment 86

[Julien Wajsberg [:julienw]]

No no, it was the QA requests from previous comments :)

Thanks Hubert, and so, the Contacts app did load after this? Which data did you use?

Comment 87

[Hubert Lu[:hlu] <[e-mail]>]

I install "reference-workload-x-heavy",2000 contacts, and the contacts are loaded properly.
If you want to know the result for 200 contacts, the result is listed in below and it also is tested in Inari.

* v1.1 to v1.3 => 12 seconds.
* v1.2 to v1.3 => 10 seconds.

Comment 88

[Julien Wajsberg [:julienw]]

Herbert, what I really want, is using real-world data. The reference workloads, I can test them myself (and I did it!).

Comment 89

[Loli (:lolimartinezcr)]

Tested 1.1 -> 1.3 on a Unagi:
1.1
Build ID: 20140303131755
Platfomr version: 18.1
Git Commint: c5cb252e5d1aa45d14f3a2ea182e73e

1.3
Build ID: 20140303045237
Platform version: 28.0
Git Commit: d51d2e09


Tested 1.2 -> 1.3 on a Unagi:
1.2
Build ID: 20140303105246
Platfomr version: 26.0
Git Commit:  539a25e1887b902b8b25038c547048e691

1.3
Build ID: 20140303045237
Platform version: 28.0
Git Commit: d51d2e09

In two cases i can see:

Prerequesites:
1. Device with v1.1 installed
2. Gmail contacts imported (in my case I have tested with 1951 contacts and in other test with 300 contacts)

STR:
1. Update via FOTA device to v1.3
2. Open the Contacts App and stay idle for about 5 seconds.

Actual result:
Initially you will see that Gmail data is not loaded. If you want to know the result for contacts,

Expected result:
Initially you can see Gmail data loaded

Reproduction Frequency: 100%
Attached: logcat.txt, logcat_Contact.txt and video

Comment 90

[Loli (:lolimartinezcr)]

Attachment: logcat.txt

Comment 91

[Loli (:lolimartinezcr)]

Attachment: logcat_Contact.txt

Comment 92

[Loli (:lolimartinezcr)]

Attachment: VID_0002.3gp

Comment 93

[Loli (:lolimartinezcr)]

(In reply to Loli from comment #89)
> Tested 1.1 -> 1.3 on a Unagi:
> 1.1
> Build ID: 20140303131755
> Platfomr version: 18.1
> Git Commint: c5cb252e5d1aa45d14f3a2ea182e73e
> 
> 1.3
> Build ID: 20140303045237
> Platform version: 28.0
> Git Commit: d51d2e09
> 
> 
> Tested 1.2 -> 1.3 on a Unagi:
> 1.2
> Build ID: 20140303105246
> Platfomr version: 26.0
> Git Commit:  539a25e1887b902b8b25038c547048e691
> 
> 1.3
> Build ID: 20140303045237
> Platform version: 28.0
> Git Commit: d51d2e09
> 
> In two cases i can see:
> 
> Prerequesites:
> 1. Device with v1.1 installed
> 2. Gmail contacts imported (in my case I have tested with 1951 contacts and
> in other test with 300 contacts)
> 
> STR:
> 1. Update via FOTA device to v1.3
> 2. Open the Contacts App and stay idle for about 5 seconds.
> 
> Actual result:
> Initially you will see that Gmail data is not loaded. If you want to know
> the result for contacts,
> 
> Expected result:
> Initially you can see Gmail data loaded
> 
> Reproduction Frequency: 100%
> Attached: logcat.txt, logcat_Contact.txt and video

Actual result:
Initially you will see that Gmail data is not loaded. If you want to know the result for contacts,you have to hope more 1 minute

Comment 94

[Loli (:lolimartinezcr)]

(In reply to Loli from comment #93)
> (In reply to Loli from comment #89)
> > Tested 1.1 -> 1.3 on a Unagi:
> > 1.1
> > Build ID: 20140303131755
> > Platfomr version: 18.1
> > Git Commint: c5cb252e5d1aa45d14f3a2ea182e73e
> > 
> > 1.3
> > Build ID: 20140303045237
> > Platform version: 28.0
> > Git Commit: d51d2e09
> > 
> > 
> > Tested 1.2 -> 1.3 on a Unagi:
> > 1.2
> > Build ID: 20140303105246
> > Platfomr version: 26.0
> > Git Commit:  539a25e1887b902b8b25038c547048e691
> > 
> > 1.3
> > Build ID: 20140303045237
> > Platform version: 28.0
> > Git Commit: d51d2e09
> > 
> > In two cases i can see:
> > 
> > Prerequesites:
> > 1. Device with v1.1 installed
> > 2. Gmail contacts imported (in my case I have tested with 1951 contacts and
> > in other test with 300 contacts)
> > 
> > STR:
> > 1. Update via FOTA device to v1.3
> > 2. Open the Contacts App and stay idle for about 5 seconds.
> > 
> > Actual result:
> > Initially you will see that Gmail data is not loaded. If you want to know
> > the result for contacts,
> > 
> > Expected result:
> > Initially you can see Gmail data loaded
> > 
> > Reproduction Frequency: 100%
> > Attached: logcat.txt, logcat_Contact.txt and video
> 
> Actual result:
> Initially you will see that Gmail data is not loaded. If you want to know
> the result for contacts,you have to hope more 1 minute

When i say "more 1 minute" is in test with 1951 contacts. This time depends of number of contacts to load.

Comment 95

[Julien Wajsberg [:julienw]]

Hey Loli,

How much time did you need to display the list of contacts? From the logcat I see it's a little more than 1 minute, were the contacts correctly displayed after that?

If yes, then to verify this bug, you'll need to do the same with a build from before this patch. I expect that the time was a lot more important.

Comment 96

[Loli (:lolimartinezcr)]

(In reply to Julien Wajsberg [:julienw] from comment #95)
> Hey Loli,
> 
> How much time did you need to display the list of contacts? From the logcat
> I see it's a little more than 1 minute, were the contacts correctly
> displayed after that?
> 
> If yes, then to verify this bug, you'll need to do the same with a build
> from before this patch. I expect that the time was a lot more important.

When i tap in contact aplication i see contacts correctly load after 1 minute (1951 contacts load).

If you think this time isn't important i verify this bug.

Comment 97

[Loli (:lolimartinezcr)]

(In reply to Loli from comment #96)
> (In reply to Julien Wajsberg [:julienw] from comment #95)
> > Hey Loli,
> > 
> > How much time did you need to display the list of contacts? From the logcat
> > I see it's a little more than 1 minute, were the contacts correctly
> > displayed after that?
> > 
> > If yes, then to verify this bug, you'll need to do the same with a build
> > from before this patch. I expect that the time was a lot more important.
> 
> When i tap in contact aplication i see contacts correctly load after 1
> minute (1951 contacts load).
> 
> If you think this time isn't important i verify this bug.


In https://bugzilla.mozilla.org/show_bug.cgi?id=951806#c0 it took over 30 minutes  now took over 1 minute, for this reason this bug in VERIFIED

Comment 98

[Julien Wajsberg [:julienw]]

Thanks Loli! It's not really possible to do less than 1 minute but bug 932893 exists to show a user feedback while that's happening.