Facebook Import OOMs and kills itself when importing 500+ contacts

VERIFIED FIXED in Firefox OS v1.3

Status

Firefox OS
Gaia::Contacts
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: nhirata, Assigned: Jose Manuel Cantera)

Tracking

({perf, regression})

unspecified
1.3 C3/1.4 S3(31jan)
ARM
Gonk (Firefox OS)
perf, regression
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g-v1.3 verified, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

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

Attachments

(3 attachments)

Created attachment 8347550 [details]
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
Created attachment 8347553 [details]
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.
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

Updated

4 years ago
Duplicate of this bug: 952690

Updated

4 years ago
blocking-b2g: --- → 1.3+
Keywords: perf, regression
Assignee: nobody → gsvelto

Updated

4 years ago
Priority: -- → P1
Whiteboard: [c=memory p= s= u=1.3]

Comment 4

4 years ago
The PM team traiged this and considers it a blocker
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
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.

Updated

4 years ago
Whiteboard: [c=memory p= s= u=1.3] → [c=memory p= s= u=1.3][MemShrink]
(Assignee)

Comment 6

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

Updated

4 years ago
Assignee: gsvelto → jmcf
(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.
(Assignee)

Updated

4 years ago
Depends on: 950881
(Assignee)

Comment 8

4 years ago
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.
Whiteboard: [c=memory p= s= u=1.3][MemShrink] → [c=memory p= s= u=1.3][MemShrink:P2]
(Assignee)

Comment 9

4 years ago
apart from the recommendation from Gabriele I believe performance and memory will benefit from fixing also bug 950881.
(Assignee)

Updated

4 years ago
Depends on: 962516, 961785

Updated

4 years ago
Whiteboard: [c=memory p= s= u=1.3][MemShrink:P2] → [c=memory p= s= u=1.3][MemShrink:P2][mwcdemo2014]
(Assignee)

Comment 10

4 years ago
Created attachment 8365102 [details]
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
Attachment #8365102 - Flags: review?(crdlc)
Comment on attachment 8365102 [details]
15686.html

Hi, the code looks good to me although there are some comments on github. Good job
Attachment #8365102 - Flags: review?(crdlc) → review+
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Keywords: verifyme
status-b2g-v1.3: --- → affected
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
Flags: needinfo?(jmcf)
(Assignee)

Comment 14

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

thanks
Flags: needinfo?(jmcf)
(Assignee)

Comment 15

4 years ago
uplifted to v1.3:

https://github.com/mozilla-b2g/gaia/commit/1561b1c8bb5e8bce90dc93a9e3e1ccb6e7b927e2
status-b2g-v1.3: affected → fixed
John Hammink, can you please take care of this one?
Flags: needinfo?(jhammink)

Comment 17

4 years ago
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(jhammink)

Updated

4 years ago
status-b2g-v1.3: fixed → verified
Keywords: verifyme
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.