950295 - Facebook Import OOMs and kills itself when importing 500+ contacts

Status: VERIFIED FIXED

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

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat.txt

1. launch contacts
2. go to settings
3. turn on Sync friends
4. log into facebook

Expected: facebook import finishes
Actual: partial import of contacts

Note:
adb shell dmesg : 
<4>[ 3402.639558] select 1287 (Communications), adj 2, size 20343, to kill
<4>[ 3402.639581] send sigkill to 1287 (Communications), adj 2, size 20343

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: about-memory-3.zip

I'm not sure if it's doing clean up after the kill to the app.  I'm seeing it out of pmem still in the logcat.  I ended up taking an about-memory after the sigkill.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Gaia      390b313a254a947d12e3cdbcde19d7d1619ff63c
Gecko     http://hg.mozilla.org/mozilla-central/rev/8b5875dc7e31
BuildID   20131213040203
Version   29.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

Comment 4

[Sri Kasetti]

The PM team traiged this and considers it a blocker

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

I've tried importing my contacts (240) and took an about:memory snapshot when importing reached ~200 contacts. Looking at it shows something very weird (I've snipped out the non-relevant parts):

65.82 MB (100.0%) -- explicit
├──23.46 MB (35.64%) -- window-objects/top(app://communications.gaiamobile.org/contacts/index.html, id=1)
│  ├──21.48 MB (32.63%) -- active
│  │  ├──15.81 MB (24.01%) -- window(app://communications.gaiamobile.org/contacts/index.html)
│  │  │  ├──14.13 MB (21.47%) -- js-compartment(app://communications.gaiamobile.org/contacts/index.html)
│  │  │  │  ├──13.50 MB (20.51%) -- objects
│  │  │  │  │  ├──12.74 MB (19.36%) -- malloc-heap
│  │  │  │  │  │  ├──12.20 MB (18.53%) ── elements/non-asm.js
│  │  ├───4.63 MB (07.04%) -- window(app://communications.gaiamobile.org/contacts/import.html?service=facebook)
│  │  │   ├──3.05 MB (04.63%) -- js-compartment(app://communications.gaiamobile.org/contacts/import.html?service=facebook)
│  │  │   │  ├──2.43 MB (03.70%) -- objects
│  │  │   │  │  ├──1.84 MB (02.79%) -- malloc-heap
│  │  │   │  │  │  ├──1.75 MB (02.66%) ── elements/non-asm.js
├──17.65 MB (26.81%) -- js-non-window
│  ├──11.57 MB (17.58%) -- zones
│  │  ├──10.35 MB (15.72%) -- zone(0x40396800)
│  │  │  ├───9.19 MB (13.96%) -- compartment([System Principal])
│  │  │  │   ├──8.09 MB (12.30%) -- objects
│  │  │  │   │  ├──6.71 MB (10.20%) -- malloc-heap
│  │  │  │   │  │  ├──6.27 MB (09.53%) ── elements/non-asm.js

The elements/non-asm.js portions keep growing during the import and they account for dynamically added object properties. So if I'm not mistaken it seems that we're dynamically *LOTS* adding properties to one or more objects. So many that we eventually run into an OOM condition. I'll have to investigate further tomorrow but I get the feeling that all the import-from-Facebook bugs might be related to this.

Comment 6

[Jose Manuel Cantera]

thanks Andrea. I can confirm that the problem is within the code that indexes FB contacts by phone number (tel_indexer.js). If no tel numbers are present the import process can work seamlessly with more than 1000 contacts. so I'm going to re implement the index in order not to create so many objects. Thus, I'm stealing the bug

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Jose M. Cantera from comment #6)
> I can confirm that the problem is within the code that
> indexes FB contacts by phone number (tel_indexer.js). If no tel numbers are
> present the import process can work seamlessly with more than 1000 contacts.
> so I'm going to re implement the index in order not to create so many
> objects. Thus, I'm stealing the bug

Thanks, I was still looking around the code but hadn't identified where it was coming from yet. I think the issue here is that you're building the tree nodes by dynamically setting their properties, effectively using them as hash-tables. This is unfortunately very expensive both from a memory and CPU POV. You're better off if you build your tree structures in a C/C++-ish style with fixed-layout objects & native arrays.

Comment 8

[Jose Manuel Cantera]

In order to fix properly this bug not only we need to reimplement the telephone index but also fix bug 950881, otherwise the performance when importing friends is comic and the risk for OOM kill higher. This is due to the fact that we are saving the index (a big tree) everytime a new friend is added.

Comment 9

[Jose Manuel Cantera]

apart from the recommendation from Gabriele I believe performance and memory will benefit from fixing also bug 950881.

Comment 10

[Jose Manuel Cantera]

Attachment: 15686.html

the telephone index has been redesigned in order to have less nodes and using arrays as much as possible instead of object properties

Comment 11

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8365102 [details]
15686.html

Hi, the code looks good to me although there are some comments on github. Good job

Comment 12

[Jose Manuel Cantera]

fixed in master: 

https://github.com/mozilla-b2g/gaia/commit/4516f0287c3f817c14aa32b8af4d577121e62240

Comment 13

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

I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 4516f0287c3f817c14aa32b8af4d577121e62240
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 14

[Jose Manuel Cantera]

We cannot uplift this one until bug 950881 is uplifted, which now depends on another uplift. 

thanks

Comment 15

[Jose Manuel Cantera]

uplifted to v1.3:

https://github.com/mozilla-b2g/gaia/commit/1561b1c8bb5e8bce90dc93a9e3e1ccb6e7b927e2

Comment 16

[Chris Kreinbring [:CKreinbring]]

John Hammink, can you please take care of this one?

Comment 17

[John Hammink]

Checked this out, and it works after two full imports of ~ 1500 contacts

Gaia      8039a5cb7519adfa81677df577f494c6a4de6140
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e5f25becc0e7
BuildID   20140221004002
Version   28.0
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013