914532 - contacts export to sdcard can OOM kill the app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Step to reproduce:

  1) Install 1000 contacts by running "make reference-workload-heavy"
  2) Open the contacts app
  3) Settings->Export->Memory Card
  4) Click "Select All" and press "Export Contacts"

The overlay gets to "exporting to sdcard" and then crashes to a white screen.  Eventually the homescreen restarts and gets displayed.

It seems pretty likely that this is due to the OOM killer.

Comment 1

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

need more information to decide. 
log trace?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: contacts.log

Here is a log of an attempted export with 1000 contacts.

Around when the contacts goes away I see some of these:

I/Gecko   ( 7000): ###!!! [Child][AsyncChannel] Error: Channel error: cannot send/recv

Then a bit later I see:

I/GonkMemoryPressure( 7000): Checking to see if memory pressure is over.
I/GonkMemoryPressure( 7000): Memory pressure is over.

Use b2g-ps in the shell shows that both Communications and Homescreen processes are gone:

root@android:/ # b2g-ps
APPLICATION      USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
b2g              root      7000  1     180972 55092 ffffffff 4005c5e0 S /system/b2g/b2g
(Preallocated a  app_7265  7265  7000  57556  18328 ffffffff 41268ed6 D /system/b2g/plugin-container

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Please see log and additional information in comment 2.  Using ni? since it looks like you marked this triaged and I wanted to make sure you saw the new info.

Thanks.

Comment 4

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

triage: focus on 902873 first before coming to this bug. leave it as koi? for now

Comment 5

[Mike Lee [:mlee]]

David,

Based on our understanding this is a crash resulting from a new 1.2 contacts feature. Since bug 902873 referenced in comment #4 is landed please assign someone to fix this.

Thanks,
fxos-perf-triage

Comment 6

[Dave Huseby [:huseby]]

Since this is only in master, we switched this to 1.3?  Kyle, what's your take?

Comment 7

[Mike Lee [:mlee]]

Disregard comment #6; mistaken bug update.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is this only blocking because it's a regression?  It doesn't seem as important as many of the other memory issues we have for 1.2.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> Is this only blocking because it's a regression?  It doesn't seem as
> important as many of the other memory issues we have for 1.2.

For perspective, this crashes with 500 contacts (reference-workload-medium) and exports successfully with 200 contacts (reference-workload-light).  This seems in the range of reasonable workloads for real people using the device as their only phone.

Export is a new feature in 1.2, so its not really a regression against 1.1.

It just seems unwise to ship a new button that we know will crash for a non-trivial number of users.  Its not good for our brand.  If nothing else we should at least error out instead of crashing.

Just my opinion.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, 1000 seemed like a lot to me.  Between 200-500 seems like a more reasonable workload though :-/

Comment 11

[Mike Lee [:mlee]]

Kyle, in light of comment #9, will you (MemShrink) raise the priority higher than P3?

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

To be honest, I don't think this is a platform issue or a leak.  There are known issues in the app that need to be addressed.  For example, the app currently reloads all contacts into memory and constructs the entire output file in memory.  This should really use a streaming approach.

There is an issue that files apparently don't support streaming.  That could be mitigated with a number of smaller files, though.  For example, 5 files with 100 contacts, etc.

Anyway, this would seem to fall more in the comms team than the platform memshrink team.

Again, just my opinion.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Mike Lee [:mlee] from comment #11)
> Kyle, in light of comment #9, will you (MemShrink) raise the priority higher
> than P3?

Yeah, ok.

If I can help here I'm more than happy to, but I don't think that MemShrink has the knowledge (c.f. comment 12) or the people to fix this ourselves.  Gabriele, is there stuff I can do that would help you here?

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #13)
> If I can help here I'm more than happy to, but I don't think that MemShrink
> has the knowledge (c.f. comment 12) or the people to fix this ourselves. 
> Gabriele, is there stuff I can do that would help you here?

No thank you, I've already debugged a number of OOM's and memory usage regressions on the Gaia side so I should be able to tackle this as soon as I have some cycles for it :)

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Great!

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Memory report while exporting 200 contacts

I've attached a memory report highlighting the issue that's happening here. In a nutshell it seems that when we're serializing the contacts we're keeping in memory:

- a string holding the whole contents of the output file
- a string holding each image associated with a contact
- the entire contents of the file

From the report:

├──35.12 MB (46.00%) -- window-objects/top(app://communications.gaiamobile.org/contacts/index.html, id=1)
│  ├──31.26 MB (40.94%) -- js-zone(0x44299000)
│  │  ├──30.61 MB (40.10%) -- strings
│  │  │  ├──30.50 MB (39.95%) -- notable
│  │  │  │  ├──18.64 MB (24.42%) -- string(length=4342703, copies=1, "BEGIN:VCARD/nVERSION:4.0/nn:Card;...)
│  │  │  │  ├───6.00 MB (07.86%) -- string(length=149264, copies=12, "photo:data:image/png;base64,iVBOR...) 
│  │  │  │  ├───2.50 MB (03.27%) -- string(length=130608, copies=10, "photo:data:image/png;base64,iVBOR...)
│  │  │  │  ├───1.75 MB (02.29%) -- string(length=46265, copies=14, "photo:data:image/jpeg;base64,/9j/4...)

And also:

├───8.00 MB (10.48%) -- dom
│   ├──8.00 MB (10.48%) ── memory-file-data/large/file(length=4342854, sha1=d117a723bba8bead3d2078375390b1f10c2db36f)

This obviously doesn't scale. I've dug up a bit in the code and found that the source of this problem is in the shared conversion code:

http://is.gd/yMeBgk

What we do there is to iterate over all contacts and serialize them together in a single string including the images. From a memory consumption perspective the serialized images shouldn't be a problem; I did a couple of tests and the GC seems to reclaim them just fine. However the string of vCards and the memory used to back the output file are an issue. If there's too many contacts they will get large enough to kill all other applications and in extreme cases the contacts app too.

I'm needinfo'ing Michal and Sergi who wrote respectively the contacts-specific and shared export-to-vCard code in hope that they have an idea how we could split this process into chunks so that we can scale to an arbitrary number of contacts without building a monster string in memory.

BTW there's another annoying issue I found out: even after the export has been completed the application is still hanging to the string of exported vCards. Forcing the GC doesn't seem to help so there must be a closure keeping it alive somehow though I couldn't find which one by just looking at the code. We might want to open a follow up to address the issue.

Comment 17

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Gabriele,

Thanks for spending time on debugging this. Indeed it looks like this doesn't scale. I will take a look at it and report back as soon as I can, hopefully with a patch. My initial thought is to do the same as in vcard import, where we process contacts by batches, freeing memory taken at each step. I need to refresh my memory about how the export is implemented though.

I will keep the needinfo to myself to remind me of posting a follow-up very soon.

Thanks.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can you get a heap dump?  We could tell from that what is keeping the string alive.

Comment 19

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi,

I am writing the streaming (i.e. batching) code for the contacts -> vcard export. The main design decision is how the consumers of the ContactToVcard will handle several calls with batches of contacts. The obvious choice would be to keep appending them to the same file in the SDCard until there are no more, and then present the user with the final file at the end.

Thoughts?

Comment 20

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #18)
> Can you get a heap dump?  We could tell from that what is keeping the string
> alive.

I'll dot that but I'd prefer to wait for Sergi to finish the batch-conversion code and then check if the problem presents itself again.

Comment 21

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Sergi Mansilla from comment #19)
> I am writing the streaming (i.e. batching) code for the contacts -> vcard
> export. The main design decision is how the consumers of the ContactToVcard
> will handle several calls with batches of contacts. The obvious choice would
> be to keep appending them to the same file in the SDCard until there are no
> more, and then present the user with the final file at the end.

Currently your code returns a large string making it the caller's responsibility to write it to a file. I think we might stick to that model except that you invoke the user-provided callback with the portion of the string associated with the current batch of contacts. Does this sound feasible? I think this should make it simpler for the existing consumers.

Also would you like to do this in this bug or open a dependent one? If you go for the former feel free to take it from me; otherwise open a new bug and put it as this bug's dependencies so we can track it separately. Thanks for your help!

(clearing the needinfos as we figured out what needs to be done already :) )

Comment 22

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Yeah, I have that almost implemented, but the problem will still be that the caller will have to hold it up in memory anyway in order to write it to a file, since the sdcard API doesn't allow to append content, only to create, read and delete.

A solution could be to create several .vcf files in the sdcard, but that starts feeling like a bit of yak shaving. Is there something I am missing API-wise or perhaps a better idea? It sounds to me like somebody must have gone through that problem before, and there must be a way to stream contents into a sdcard file.

Comment 23

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

The change for batching contact export is in progress here, but we will still have the problem of storing it to a single file in sdcard. The two files using the export functionality are bt.js and sd.js.

https://github.com/comoyo/gaia/compare/sdcard-oom?expand=1#diff-1

Comment 24

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Sergi Mansilla from comment #22)
> Yeah, I have that almost implemented, but the problem will still be that the
> caller will have to hold it up in memory anyway in order to write it to a
> file, since the sdcard API doesn't allow to append content, only to create,
> read and delete.

I'm not familiar with the DeviceStorage API yet but a quick look at the documentation shows that we could open an editable file with DeviceStorage.getEditable():

https://developer.mozilla.org/en-US/docs/Web/API/DeviceStorage.getEditable

Then use FileHandle.open() to make it writable:

https://developer.mozilla.org/en-US/docs/Web/API/FileHandle.open

And finally append to it via LockedFile.append():

https://developer.mozilla.org/en-US/docs/Web/API/LockedFile.append

A quick search doesn't show any part of Gaia using them so I wonder if we're supposed to use them or not. Still they seem to be available in Gecko 26 and I don't see any other options so I'll try and create a patch once you have batchified the vCard conversion code.

Comment 25

[Mike Lee [:mlee]]

Updating Target Milestone for FxOS Perf koi+'s.

Comment 26

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Attachment: Github PR

This patch batchifies exporting contacts. It doesn't solve the problem, it just moves it to the API consumers, such as sd.js or bt.js, who still have to store the vcard string, but this could be handled by the solution Gabriele proposes above.

Note that the batches are not sequential now. The exporter keeps processing batches and doesn't wait for the consumers to give any signal about the processing of the previous batch being finalized. This would involve the API consumers to somehow signal the exporter that it can go on with the next batch, probably using a callback. I'd like your opinion on whether that's necessary or not.

Comment 27

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 825803 [details] [diff] [review]
Github PR

I'm passing the review to Anthony because I'm no dialer peer so I cannot review the patch. I can give you a few suggestions though:

- The code in ContactsBTExport saves the vCards into a file and then sends it via an activity; the first step is basically the same as what ConvertSDExport does. Instead of having this duplicate code it might be worth unifying it and having ContactsBTExport rely on ConvertSDExport for the first step

- The ContactToVCard() batched conversion code now calls processContact() calls itself recursively, then calls sendAndNextBatch() recursively which then calls processBatch() starting the process over. If I understand the code correctly this will mean that we'll have at least one recursive call per contact which will consume quite a bit of stack memory and might even cause other objects to be retained for longer than necessary. I would suggest to change this into some straight-line code that iterates over the contacts, for example (pseudo-code):

ContactToVcard = function(ctArray, callback) {
  var vCardString = '';
  var cardsInBatch = 0;
  var batchSize = 10;

  for (var i = 0; i < ctArray.length; i++) {
    // Process a contact and append it to this batch
    vCardString += processContact(ctArray[i]);
    cardsInBatch++;

    if (cardsInBatch == batchSize) {
      // Once we've filled the batch pass it to the callback
      callback(vCardString);
      cardsInBatch = 0;
      vCardString = '';
    }
  }

  // Pass the last batch to the caller and we're done
  callback(vCardString);
}

Comment 28

[Anthony Ricaud (:rik)]

Comment on attachment 825803 [details] [diff] [review]
Github PR

Contacts is not any more part of the Dialer component so redirecting to a Contacts peer.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 825803 [details] [diff] [review]
Github PR

Sergi, can you rebase the pull request against most recent master?  It appears to have bit rotted.  I tried manually merging it, but the code is failing and I'm not sure if its a bad merge on my part.

The error I am getting is:

E/GeckoConsole(13037): [JavaScript Error: "TypeError: Argument 1 of DeviceStorage.addNamed is not an object." {file: "app://communications.gaiamobile.org/contacts/js/export/sd.js" line: 79}]

This occurs when I try to export even a single contact on gaia master with a mozilla-central from around 11/1/2013.

Also, I agree with what Gabrielle said in comment 27.  If we are concerned with memory we should really favor a normal loop structure instead of recursion.

Based on these issues I'd like to see it again.  Please flag me again when you've had time to update the code.  Thanks for tackling this one!

Also, are you planning to work on a patch using DeviceStorage.getEditable() as proposed comment 24?  I'm just curious what the overall plan is.  Feel free to email me or ping me in IRC.

Thanks again!

Comment 30

[Gabriele Svelto [:gsvelto] (PTO)]

Since I've cleared my backlog I can pick this up myself and complete the changes Sergi started and add the append-to-file changes that should solve the problem. Sergi, what do you think?

Comment 31

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi Gabriele,

I started working on your suggestions on the vcard-importer. What do you think about you focusing on sd/bt doing the append-to-file and I finish improving vcard-importer? You can assume the existing interface will not change.

Let me know what you think about this.

Comment 32

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Sergi Mansilla from comment #31)
> What do you
> think about you focusing on sd/bt doing the append-to-file and I finish
> improving vcard-importer? You can assume the existing interface will not
> change.

Sounds good, I'll write a follow-up patch based on the current interface and post it here shortly.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Thanks guys!

I also just wanted to mention that another potential area of improvement would be before the ContactToVcard() call.  Currently the List.selectFromList() function provides the entire list of contact IDs in one array.  The settings code then passes this monolithic block to the export code which loads all of the contacts into memory.  These are then sent to ContactToVcard().

It would be nice to make this an incremental process all the way through from selection to export to vcard conversion to file.  I had some code to help with the selectFromList() to settings part that I could resurrect, but we would still need to refactor export to use a streaming approach as well.

Just throwing that out there in case the memory problems persist.

Thanks!

Comment 34

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ben Kelly [:bkelly] from comment #33)
> It would be nice to make this an incremental process all the way through
> from selection to export to vcard conversion to file.

Thanks for the tip, it would be better indeed and probably make this scale to an arbitrary number of contacts. I'm actually wondering if this would fix another problem I'm seeing. I could reproduce the OOM here with 500 contacts but not with a thousand. In that case the spinner keeps going on and on forever and the conversion process never starts. If contacts.List.selectFromList() returns all the contacts *with* their pictures already loaded in memory it might be possible that this is enough to make the process fail but without triggering an OOM (make because of a fallible path in the Gecko code?); in that case processing a chunk at a time as you suggest would be the only solution.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

The old code I wrote to provide incremental contact results from the list selection is in my contacts-select-perf branch on github.  It has a user-space cursor object:

  https://github.com/wanderview/gaia/blob/contacts-select-perf/apps/communications/contacts/js/utilities/cursor.js

Its created and used by the list code here:

  https://github.com/wanderview/gaia/blob/contacts-select-perf/apps/communications/contacts/js/views/list.js#L1282

But I aggregated the list again in settings since export would still need to change:

  https://github.com/wanderview/gaia/blob/contacts-select-perf/apps/communications/contacts/js/views/settings.js#L256

An added benefit of this approach is it also reuses contacts already cached in the list code if they are present.

Comment 36

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Thanks Ben, that sounds like a great idea. I updated my branch some minutes ago, btw. It is now synced with master and using iteration instead of recursion.

Comment 37

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Should we create a separate issue to provide incremental contacts to export, then? Gabriele, did you manage to make the changes to bt/sd?

Comment 38

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Sergi Mansilla from comment #37)
> Should we create a separate issue to provide incremental contacts to export,
> then?

Yes please, open a follow-up bug for that.

> Gabriele, did you manage to make the changes to bt/sd?

Not yet but I'm working on them.

Comment 39

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2] Incrementally write vCards to disk when exporting contacts on the sdcard

This is a work-in-progress patch that modifies the exporting code to progressively write vcards to disk. It's not working yet unfortunately.

Comment 40

[Gabriele Svelto [:gsvelto] (PTO)]

I've modified Sergi's patch to execute the conversion in an asynchronous yet purely iterative way. It passes the unit-tests but I haven't had time to test it in the actual usage scenarios. It also still needs some polish; for example I intend to drop the ContactToVCardBlob() function as it doesn't make much sense now that we convert in batches. I will modify its consumers to use the batched ContactToVCard instead.

Comment 41

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2] Make ConvertToVCard process contacts in batches

Since the whole fix is quite large I've decided to split up the patches to simplify review. In this first patch I only modified the ConvertToVCard() function to work in batches and adjusted the relevant unit-tests accordingly. With this patch applied the export code using ConvertToVCard() is thus broken as those changes will go into the following patch. Note that I did a few additional adjustments:

- I've removed the ContactToVCardBlob() convenience function as it didn't make sense anymore and it can be trivially replaced if need be

- I've used Date.toISOString() to produce ISO 8601-compliant dates

- I've fully encapsulated the code in contact2vcard.js so that it now exports only the ContactToVCard() function and no other function/variable

- I've overhauled the unit-tests so that they now do all the assertions within the done() function; in case of errors this ensures that proper stack traces are printed out with the correct location of the assertion that failed

- I've cleaned up both contact2vcard.js and its unit-tests so that they are now hint-clean

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Gabriele, would it be possible to have the two patches as separate commits within a single pull request?  That would let us land/uplift/revert any changes atomically using the merge commit.

Comment 43

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ben Kelly [:bkelly] from comment #42)
> Gabriele, would it be possible to have the two patches as separate commits
> within a single pull request?  That would let us land/uplift/revert any
> changes atomically using the merge commit.

Yes, I intended to merge the patches that way. I'm just keeping them separated here for ease of review.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Ok, thanks!  I will most likely review this tomorrow morning.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 830927 [details] [diff] [review]
[PATCH 1/2] Make ConvertToVCard process contacts in batches

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

Overall looks good.  A few nits, some error checking, and would like to see a test that exercises the batching.  r=me with those changes.

::: apps/communications/contacts/test/unit/contact2vcard_test.js
@@ +36,5 @@
> +      ContactToVcard([mc], function append(vcards, nCards) {
> +        str += vcards;
> +        count += nCards;
> +      }, function success() {
> +        done(function() {

What does passing a function to |done()| do?  Run it within a try/catch?

@@ +62,5 @@
>      });
>  
>      test('Convert multiple contacts to a vcard', function(done) {
>        var mc = new MockContactAllFields();
> +      var contacts = [mc, new mozContact()];

This won't actually exercise the batching logic.  Can you expand this test or write a new test that will trigger multiple batches?  Might be easier if the batch size could be passed in as an optional argument.

@@ +104,5 @@
> +        count += nCards;
> +      }, function success() {
> +        assert.equal(count, contacts.length);
> +        assert.strictEqual(str, '');
> +        done();

Why does this test not following the convention used in your prior tests of |done(function() {})|?

::: shared/js/contact2vcard.js
@@ +135,5 @@
> +   *        representing the number of contacts in the string.
> +   * @param {Function} success A function with no parameters that will be
> +   *        invoked once all the contacts have been processed.
> +   */
> +  function ContactToVcard(contacts, append, success) {

This function assumes |append| and |success| are always passed as functions.  Since this can blow up on the user, we probably need to verify these arguments.  If these are always required, we should probably throw immediately if |typeof append !== 'function'| instead of dying later in the async pump.

@@ +137,5 @@
> +   *        invoked once all the contacts have been processed.
> +   */
> +  function ContactToVcard(contacts, append, success) {
> +    var vCardsString = '';
> +    var i = 0;

Nit: This variable is used for persistent state across multiple calls to appendVCard.  Can we use a more descriptive name like "nextIndex" or "nextContactIndex"?  Don't want to be too verbose, but single letter is probably a bit too brief here.

@@ +139,5 @@
> +  function ContactToVcard(contacts, append, success) {
> +    var vCardsString = '';
> +    var i = 0;
> +    var cardsInBatch = 0;
> +    var maxBatchSize = 1024 * 1024;

Nit: Perhaps a comment here indicating the limit is on number of characters in the resulting string vs number of contacts.

Comment 46

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v2] Make ConvertToVCard process contacts in batches

Thanks for the review! Here's the updated patch, I've asked for review again because I've added back the ConvertToVcardBlob() convenience function. The reason I did so is that it's needed for the Bluetooth export; since I hadn't looked at the code I hadn't realized that making that code work in batches made no sense. This also reduces the amount of changes in the second patch.

(In reply to Ben Kelly [:bkelly] from comment #45)
> What does passing a function to |done()| do?  Run it within a try/catch?

I'm not sure how it's implemented but if an assertion fails the stack-trace shows the exact place where it happened. Assertions failing within an asynchronous functions on the other hand show unhelpful stack-traces so in general doing your checks within done() makes the unit-test easier to debug when things break.

> This won't actually exercise the batching logic.  Can you expand this test
> or write a new test that will trigger multiple batches?  Might be easier if
> the batch size could be passed in as an optional argument.

I've added an optional batch-size argument to ConvertContactToVcard() and I've ensured that the unit test generates at least two batches.

> Why does this test not following the convention used in your prior tests of
> |done(function() {})|?

I forgot to, fixed in this patch.

> This function assumes |append| and |success| are always passed as functions.
> Since this can blow up on the user, we probably need to verify these
> arguments.  If these are always required, we should probably throw
> immediately if |typeof append !== 'function'| instead of dying later in the
> async pump.

Good point, I've added a check for both functions.

> Nit: This variable is used for persistent state across multiple calls to
> appendVCard.  Can we use a more descriptive name like "nextIndex" or
> "nextContactIndex"?  Don't want to be too verbose, but single letter is
> probably a bit too brief here.

Indeed, and in fact it was shadowed by another function called "i" deep within the nested functions which is not good at all. I've renamed it to "nextIndex", thanks for your suggestion.

> Nit: Perhaps a comment here indicating the limit is on number of characters
> in the resulting string vs number of contacts.

I've added a comment in the parameters description.

Comment 47

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2 v2] Adjust the SD card export code to write out the contacts incrementally

This is the second part of the patch, it modifies the SD card export code to stream out the converted vCards instead of writing all of them as a single blob.

I'm not asking for review just yet because while the code does pass the unit tests I haven't had the time to test it on a device yet. Since from what I can tell it's the very first code in Gaia using DeviceStorage.getEditable() and the associated functionality it might be horribly broken.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 831643 [details] [diff] [review]
[PATCH 1/2 v2] Make ConvertToVCard process contacts in batches

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

Looks good!  Just a couple nits and a test question.  r=me

::: apps/communications/contacts/test/unit/contact2vcard_test.js
@@ -31,5 @@
> -      var d2 = new Date('January 02, 1981 9:24:00 +4');
> -      assert.strictEqual(ISODateString(d2), '1981-01-02T05:24:00Z');
> -      done();
> -    });
> -

Why do we no longer test our date conversion?  I realize we use a standard function internally now, but I think it would be good to maintain the test to avoid future regressions.

::: shared/js/contact2vcard.js
@@ +137,5 @@
> +  function ContactToVcardBlob(contacts, callback) {
> +    var str = '';
> +
> +    ContactToVcard(contacts, function append(vcards, nCards) {
> +      str += vcards;

I was about to suggest we use Array.join() instead of string concatenation here, but I see now that browsers heavily optimize string concat and its actually faster.

@@ +170,4 @@
>  
> +    if (batchSize && (batchSize > 0)) {
> +      maxBatchSize = batchSize;
> +    }

Nit:  I think it might be more idiomatic to write this as:

  var matchBatchSize = batchSize || (1024 * 1024);

Or even:

  batchSize = batchSize || (1024 * 1024);

@@ +175,5 @@
> +    if (!append || (typeof append !== 'function')) {
> +      throw Error('append() is undefined or not a function');
> +    }
> +
> +    if (!success || (typeof success !== 'function')) {

Nit: You don't need the |!append| or |!success| checks here.  The variable can't be a falsy value and also pass the the |typeof success !== 'function'| check.

Comment 49

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2 v3] Adjust the SD card export code to write out the contacts incrementally

This is a complete version of the stream-out exporting code. Unfortunately it doesn't work yet because the functionality we need (DeviceStorage.getEditable()) has not been implemented yet. See bug 859696.

I'm trying to figure out if we can get that functionality in time otherwise we'll have to recur to some ugly workaround. One possibility would be to write out the contacts in multiple files.

Comment 50

[Gabriele Svelto [:gsvelto] (PTO)]

After discussing with :dhylands and :bkelly I've reached the conclusion that there's no possible way of fixing the issue at hand within the 1.2 timeframe simply because we don't have the necessary functionality to stream out writes to a file. With the current APIs we can only create a file as a whole blob which limits us pretty severely (files larger than 1/3 of the available memory are sure to cause OOMs).

So the only alternative we have to fix this bug in a reasonable amount of time is to force the export code to split up the output in multiple files of a predefined size. Francisco, do you think this would be acceptable for this bug? We would obviously open a follow up to fix the issue once the required functionality lands in the platform.

Comment 51

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I see. If we decide to go ahead with the multiple files solution I'd be happy to take the issue.

Comment 52

[Alex Keybl [:akeybl]]

(In reply to Gabriele Svelto [:gsvelto] from comment #50)
> After discussing with :dhylands and :bkelly I've reached the conclusion that
> there's no possible way of fixing the issue at hand within the 1.2 timeframe
> simply because we don't have the necessary functionality to stream out
> writes to a file. With the current APIs we can only create a file as a whole
> blob which limits us pretty severely (files larger than 1/3 of the available
> memory are sure to cause OOMs).
> 
> So the only alternative we have to fix this bug in a reasonable amount of
> time is to force the export code to split up the output in multiple files of
> a predefined size. Francisco, do you think this would be acceptable for this
> bug? We would obviously open a follow up to fix the issue once the required
> functionality lands in the platform.

Triage agrees that we should either do this or make sure we don't OOM crash and list this as a known 1.2 issue in the notes ("Only the first X contacts will be exported").

Comment 53

[Francisco Jordano [:arcturus] [:francisco]]

:gsvelto not having a proper streaming api for writing to disk, seems to me like the only feasible solution.

+1 to Gabrielle solution.

Comment 54

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2 v4] Split contacts exported to the SD card if the output is too large

Thanks Francisco, this is a modified patch that splits the output files in 2MiB batches. I've tested it with the heavy reference workload (1000 contacts) and it works well on my hamachi, splitting the output in 9 files. Larger batches wouldn't work, I had to do some trial and error to find the right sport:

- 8MiB batches 200 contacts worked, 500 failed
- 4MiB batches 500 contacts worked, 1000 failed
- 2MiB batches 1000 contacts worked

I'll post an updated first patch with the review comments from comment 48 addressed soon then once this is also reviewed I'll squash both patches into a single commit and push it to master.

Going forward we need a follow-up bug to fix the outstanding issues:

- Output to a single file once we have append functionality
- Retrieve contacts incrementally as described in comment 35

Comment 55

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v3] Make ConvertToVCard process contacts in batches r=bkelly

Updated patch with all nits addressed, carrying over the r=bkelly flag. Note that I have not added back the date conversion unit-test as the code wasn't exposed publicly anymore but I've made the vcard output date check stricter which effectively achieves the same purpose.

Comment 56

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Split contacts exported to the SD card into multiple files if the output is too large

Comment 57

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Thanks for this work Gabriele!

Is there a particular issue open for on append-functionality? Looks like it should have priority, since determining file size by trial-and-error on a particular phone model sounds a bit risky (not that I can come up with a better method right now), and probably we'll need append functionality in other areas more and more from now on.

Comment 58

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Sergi Mansilla from comment #57)
> Thanks for this work Gabriele!

You're welcome :)

> Is there a particular issue open for on append-functionality?

Yes, it's being tracked in bug 859696.

> Looks like it
> should have priority, since determining file size by trial-and-error on a
> particular phone model sounds a bit risky (not that I can come up with a
> better method right now), and probably we'll need append functionality in
> other areas more and more from now on.

Indeed, we should prioritize this.

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8334561 [details] [diff] [review]
[PATCH 2/2 v4] Split contacts exported to the SD card if the output is too large

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

Looks good!  Just check that we load setImmediate.js in the bluetooth case.  With that, r=me.

::: apps/communications/contacts/js/views/settings.js
@@ +240,5 @@
>            [
>              '/shared/js/device_storage/get_storage_if_available.js',
>              '/shared/js/device_storage/get_unused_filename.js',
>              '/shared/js/contact2vcard.js',
> +            '/shared/js/setImmediate.js',

Don't we need to load this for the bluetooth case as well?

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8334570 [details] [diff] [review]
[PATCH 1/2 v3] Make ConvertToVCard process contacts in batches r=bkelly

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

::: shared/js/contact2vcard.js
@@ +133,5 @@
> +   *
> +   * @param {Array} contacts An array of mozContact objects.
> +   * @param {Function} callback A function invoked with the generated blob.
> +   */
> +  function ContactToVcardBlob(contacts, callback) {

I know I already r+'d this patch, but can we add a check to make sure |callback| is a function here?  I think this has the same problem the |ContactToVcard| entry point had where we blow up later in an async context if |callback| is bad.

Thanks!

Comment 61

[Gabriele Svelto [:gsvelto] (PTO)]

Thanks for the review!

(In reply to Ben Kelly [:bkelly] from comment #59)
> Don't we need to load this for the bluetooth case as well?

Yup, nice catch. That would have been an unpleasant regression.

(In reply to Ben Kelly [:bkelly] from comment #60)
> I know I already r+'d this patch, but can we add a check to make sure
> |callback| is a function here?  I think this has the same problem the
> |ContactToVcard| entry point had where we blow up later in an async context
> if |callback| is bad.

Yes, it's no problem at all. I'll squash both patches into one and address this issue too.

Comment 62

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v5] Split contacts exported to the SD card if the output is too large r=bkelly

Here's the unified patch, it addresses all remaining review comments so I'm carrying over the r=bkelly flag. I've given it a final spin on my device and it's working fine; Travis is also green so I'm going to push this to master right away.

Comment 63

[Gabriele Svelto [:gsvelto] (PTO)]

Pushed to gaia/master 50b584f1f178f4a8a2dbd572eabafbb004a83b25

The patch needs some minor adjustments to apply cleanly to v1.2 so I'm testing it first. I've set the NO_UPLIFT flag as I'll provide the revised patch as soon as I'm done with the testing.

Comment 64

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH gaia/v1.2] Split contacts exported to the SD card if the output is too large r=bkelly

Uplifted patch, I've just tested it and it works fine. The changes were minimal so I'm carrying over the r=bkelly flag.

Comment 65

[Gabriele Svelto [:gsvelto] (PTO)]

Pushed attachment 8335359 [details] [diff] [review] to gaia/v1.2 23076d88899fc44bd658eef932d5d707ae621a36