Closed Bug 783720 Opened 12 years ago Closed 12 years ago

OOM when loading 19 .mp3 files in gaia music app

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-basecamp:+)

Status:
RESOLVED INCOMPLETE
Project Flags:
blocking-basecamp +

People

(Reporter: cjones, Unassigned)

Details

(Keywords: dogfood)

Attachments

(1 file)

GC log
13.67 KB, text/plain
Details 
I added 19 .mp3 files to /sdcard/Music/[Artist]/[Album].  When I launch the music app, those files don't show up immediately, but I'm pretty sure they're being indexed because soon thereafter, the b2g process crashes from OOM.  (Confirmed in gdb: receiving SIGKILL.)

The files were purchased (really, I swear! ;) ) from amazonmp3, and are 256kbps so fairly big.  They also have pretty high-res album art.

I'd like to know why we're OOM'ing on so few files.  We shouldn't have to read much data before hitting the end of the metadata.  Blame probably lies across gaia/gecko here.

If I just have one file from the album in /sdcard/Music/[file], then it loads great!  Album art and everything.  +1
Need to figure out what's up here --- can't crash when loading 20 files.
blocking-basecamp: --- → +
Do you have the changes from bug 782694?
Should, but I'll rebuild to double check.
Attached file GC log — 
Yep.  GC/CC log attached.  I can't tell from the log how much memory we're collecting, but it almost looks like we're reading entire files into RAM ...
Comment on attachment 653006 [details]
GC log

A TOO_MUCH_MALLOC GC is triggered whenever we dynamically allocate 16MB after my GC parameter change. After a GC the counter is set to 0 again. Indeed we allocate a lot of stuff here.
I'm pretty sure that we're seeing the same issue with Gallery (I'm seeing OOM in gallery as well).
Reading one into RAM might be ok, but all 19?
(In reply to Dave Hylands [:dhylands] from comment #6)
> I'm pretty sure that we're seeing the same issue with Gallery (I'm seeing
> OOM in gallery as well).

Slightly different issue here, since we're just scanning metadata.  We have to load entire images to display them.  And the audio files are 6-10MB each :/.
Took a quick look at $gaia/apps/music/js/ext/id3-minified.js, and it looks like that code depends on the entire file being read into memory :(.  We need to fix that.
Do we OOM because of one file? Or do they all stick around in memory?
Beside reading the whole file, it reads the file as binary string instead of array buffer...

https://github.com/mozilla-b2g/gaia/issues/2277
(In reply to Andreas Gal :gal from comment #10)
> Do we OOM because of one file? Or do they all stick around in memory?

It sort of doesn't matter, because when a user uploads a 50MB lecture or comedy routine we'll crash if we do things like this.
Does FileHandle implemented in bug 726593 gives us the ability to read arbitrarily segment of the file, or we would need to wait for bug 752724? Specifically, can we get a FileHandle of a file in Device Storage for reading *today*?
I just checked to verify that shared/js/mediadb.js does not attempt parse metadata in parallel for new files.  So it should only be doing one of the 19 new mp3s at a time.

Switching to an array buffer ought to improve memory usage a lot.

If we can't use FileHandle, we could use blob.slice() to avoid reading everything, I think.  We'd need to be sure that the gecko implementation actually slices before loading the entire parent blob into memory, though.

For this and other media app OOM errors, it would be nice to get an exception.  Is there anyway that process_container could trigger a JS exception so we get a line number in logcat output before it kills the process?  Why does the process have to die, anyway? Can't the JavaScript operations that are using too much memory just fail?
(In reply to David Flanagan [:djf] from comment #14)
> If we can't use FileHandle, we could use blob.slice() to avoid reading
> everything, I think.

FileHandle might not be around forever (we should try to implement a streaming protocol for file data after v1), so I'd suggest trying to do this with Blob instead.

> We'd need to be sure that the gecko implementation
> actually slices before loading the entire parent blob into memory, though.

blob.slice() sounds like exactly what you want here, assuming you know the start/end position of the metadata you care about. No data is read into memory until you pass blob objects to FileReader/FileReaderSync.
We don't know where the end of the metadata is, a priori.

Is there no way to do something like
  int fd = open("foo.mp3");
  size_t len;
  char buf[4096];
  while (len = read(fd, buf))
    // do stuff

?  Where we reuse the same fd for the read?  (Properly async'ified, of course.)
Well, you can do the same thing by slicing the blob into 4096 byte chunks and reading the whole slice. If you need more you just slice again with an offset.
And we end up reusing the same fd etc?
No, I'm pretty sure each slice() call will create a new (duplicated) fd.
Sorry if I misunderstood anything. Should we fix this in Gaia rather than Gecko/Platform, right?
Yes, the biggest fix we can apply right now is to not read each entire file into memory.  This can only be done in Gaia.

Since we don't know of anything /yet/ that gecko is doing dumbly, I'll go ahead and close this.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: