contacts export to sdcard can OOM kill the app

RESOLVED FIXED in Firefox OS v1.2

Status

P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bkelly, Assigned: gsvelto)

Tracking

({perf})

unspecified
1.2 C5(Nov22)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:koi+, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)

Details

(Whiteboard: [c=memory p= s= u=1.2] [MemShrink:P2])

Attachments

(5 attachments, 8 obsolete attachments)

108.57 KB, text/plain
Details
54.65 KB, application/x-gtar-compressed
Details
46 bytes, text/x-github-pull-request
Details | Review | Splinter Review
31.68 KB, patch
Details | Diff | Splinter Review
31.69 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
blocking-b2g: --- → koi?
(Reporter)

Updated

5 years ago
Depends on: 902873
need more information to decide. 
log trace?
Flags: needinfo?(bkelly)
Keywords: perf
Whiteboard: [comms-triaged]
(Reporter)

Comment 2

5 years ago
Created attachment 812069 [details]
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
Flags: needinfo?(bkelly)
(Reporter)

Comment 3

5 years ago
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.
Flags: needinfo?(jcheng)
triage: focus on 902873 first before coming to this bug. leave it as koi? for now
Flags: needinfo?(jcheng)

Updated

5 years ago
Whiteboard: [comms-triaged] → [c=memory p= s= u=] [MemShrink]

Comment 5

5 years ago
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
blocking-b2g: koi? → koi+
Flags: needinfo?(dscravaglieri)
Priority: -- → P1
Whiteboard: [c=memory p= s= u=] [MemShrink] → [c=memory p= s= u=1.2] [MemShrink]
Since this is only in master, we switched this to 1.3?  Kyle, what's your take?
blocking-b2g: koi+ → 1.3?
Flags: needinfo?(khuey)
Priority: P1 → --
Whiteboard: [c=memory p= s= u=1.2] [MemShrink] → [c=memory p= s= u=] [MemShrink]

Comment 7

5 years ago
Disregard comment #6; mistaken bug update.
blocking-b2g: 1.3? → koi+
Flags: needinfo?(khuey)
Whiteboard: [c=memory p= s= u=] [MemShrink] → [c=memory p= s= u=1.2] [MemShrink]

Updated

5 years ago
Priority: -- → P1
Assignee: nobody → gsvelto

Updated

5 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → 1.2 C3(Oct25)
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.
Flags: needinfo?(mlee)
Whiteboard: [c=memory p= s= u=1.2] [MemShrink] → [c=memory p= s= u=1.2] [MemShrink:P3]
(Reporter)

Comment 9

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Ok, 1000 seemed like a lot to me.  Between 200-500 seems like a more reasonable workload though :-/

Updated

5 years ago
Flags: needinfo?(mlee)
Flags: needinfo?(dscravaglieri)

Comment 11

5 years ago
Kyle, in light of comment #9, will you (MemShrink) raise the priority higher than P3?
Flags: needinfo?(khuey)
(Reporter)

Comment 12

5 years ago
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.
(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?
Flags: needinfo?(khuey) → needinfo?(gsvelto)
Whiteboard: [c=memory p= s= u=1.2] [MemShrink:P3] → [c=memory p= s= u=1.2] [MemShrink:P2]
(Assignee)

Comment 14

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 :)
Flags: needinfo?(gsvelto)
Target Milestone: 1.2 C3(Oct25) → 1.2 C4(Nov8)
(Assignee)

Comment 16

5 years ago
Created attachment 823274 [details]
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.
Flags: needinfo?(sergi.mansilla)
Flags: needinfo?(mbudzynski)
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.
Can you get a heap dump?  We could tell from that what is keeping the string alive.
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?
(Assignee)

Comment 20

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
(Assignee)

Comment 21

5 years ago
(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 :) )
Flags: needinfo?(sergi.mansilla)
Flags: needinfo?(mbudzynski)
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.
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
(Assignee)

Comment 24

5 years ago
(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

5 years ago
Updating Target Milestone for FxOS Perf koi+'s.

Updated

5 years ago
Priority: P1 → P2
Created attachment 825803 [details] [diff] [review]
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.
Attachment #825803 - Flags: review?(gsvelto)
(Assignee)

Comment 27

5 years ago
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);
}
Attachment #825803 - Flags: review?(gsvelto) → review?(anthony)
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.
Attachment #825803 - Flags: review?(anthony) → review?(bkelly)
(Reporter)

Comment 29

5 years ago
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!
Attachment #825803 - Flags: review?(bkelly) → review-
(Assignee)

Comment 30

5 years ago
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?
Flags: needinfo?(sergi.mansilla)
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.
Flags: needinfo?(sergi.mansilla)
(Assignee)

Comment 32

5 years ago
(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.
(Reporter)

Comment 33

5 years ago
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!
(Assignee)

Comment 34

5 years ago
(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.
(Reporter)

Comment 35

5 years ago
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.
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.
Should we create a separate issue to provide incremental contacts to export, then? Gabriele, did you manage to make the changes to bt/sd?
Flags: needinfo?(gsvelto)
(Assignee)

Comment 38

5 years ago
(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.
Flags: needinfo?(gsvelto)
(Assignee)

Comment 39

5 years ago
Created attachment 828736 [details] [diff] [review]
[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.
Target Milestone: 1.2 C4(Nov8) → 1.2 C5(Nov22)
(Assignee)

Comment 40

5 years ago
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.
(Assignee)

Comment 41

5 years ago
Created attachment 830927 [details] [diff] [review]
[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
Attachment #825803 - Attachment is obsolete: true
Attachment #830927 - Flags: review?(bkelly)
(Reporter)

Comment 42

5 years ago
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.
(Assignee)

Comment 43

5 years ago
(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.
(Reporter)

Comment 44

5 years ago
Ok, thanks!  I will most likely review this tomorrow morning.
(Reporter)

Comment 45

5 years ago
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.
Attachment #830927 - Flags: review?(bkelly) → review+
(Assignee)

Comment 46

5 years ago
Created attachment 831643 [details] [diff] [review]
[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.
Attachment #830927 - Attachment is obsolete: true
Attachment #831643 - Flags: review?(bkelly)
(Assignee)

Comment 47

5 years ago
Created attachment 831651 [details] [diff] [review]
[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.
Attachment #828736 - Attachment is obsolete: true
(Reporter)

Comment 48

5 years ago
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.
Attachment #831643 - Flags: review?(bkelly) → review+
(Assignee)

Comment 49

5 years ago
Created attachment 832262 [details] [diff] [review]
[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.
Attachment #831651 - Attachment is obsolete: true
(Assignee)

Comment 50

5 years ago
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.
Flags: needinfo?(francisco.jordano)
I see. If we decide to go ahead with the multiple files solution I'd be happy to take the issue.
(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").
:gsvelto not having a proper streaming api for writing to disk, seems to me like the only feasible solution.

+1 to Gabrielle solution.
Flags: needinfo?(francisco.jordano)
(Assignee)

Comment 54

5 years ago
Created attachment 8334561 [details] [diff] [review]
[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
Attachment #832262 - Attachment is obsolete: true
Attachment #8334561 - Flags: review?(bkelly)
(Assignee)

Comment 55

5 years ago
Created attachment 8334570 [details] [diff] [review]
[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.
Attachment #831643 - Attachment is obsolete: true
(Assignee)

Comment 56

5 years ago
Created attachment 8334572 [details] [review]
[PULL REQUEST] Split contacts exported to the SD card into multiple files if the output is too large
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.
(Assignee)

Comment 58

5 years ago
(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.
(Reporter)

Comment 59

5 years ago
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?
Attachment #8334561 - Flags: review?(bkelly) → review+
(Reporter)

Comment 60

5 years ago
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!
(Assignee)

Comment 61

5 years ago
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.
(Assignee)

Comment 62

5 years ago
Created attachment 8335351 [details] [diff] [review]
[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.
Attachment #8334561 - Attachment is obsolete: true
Attachment #8334570 - Attachment is obsolete: true
(Assignee)

Comment 63

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → affected
Resolution: --- → FIXED
Whiteboard: [c=memory p= s= u=1.2] [MemShrink:P2] → [c=memory p= s= u=1.2] [MemShrink:P2] [NO_UPLIFT]
(Assignee)

Comment 64

5 years ago
Created attachment 8335359 [details] [diff] [review]
[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.
(Assignee)

Comment 65

5 years ago
Pushed attachment 8335359 [details] [diff] [review] to gaia/v1.2 23076d88899fc44bd658eef932d5d707ae621a36
status-b2g-v1.2: affected → fixed
Whiteboard: [c=memory p= s= u=1.2] [MemShrink:P2] [NO_UPLIFT] → [c=memory p= s= u=1.2] [MemShrink:P2]
You need to log in before you can comment on or make changes to this bug.