New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chromium crash when writing a large memfs file #7630

Closed
Beuc opened this Issue Dec 8, 2018 · 16 comments

Comments

Projects
None yet
4 participants
@Beuc
Copy link
Contributor

Beuc commented Dec 8, 2018

Chromium 70 displays a "Aw, Snap!" when writing a file >= 108MB.

#include <stdio.h>
#include <stdlib.h>

#define BUFSIZE 1024*1024

int main(void) {
  FILE* f = fopen("/archive.rpa", "w");
  unsigned char buf[BUFSIZE] = "";
  for (int i = 0; i < BUFSIZE; i++) {
    buf[i] = rand() % 256;
  }
  for (int i = 0; i < 150; i++) {  // fails at 108 for me
    fwrite(buf, BUFSIZE, 1, f);
  }
  printf("Wrote file.\n");
}

This is consistent whatever the memory options I provide:

$ emcc ../testlarge.c -o testlarge.html -s TOTAL_MEMORY=1GB -s ALLOW_MEMORY_GROWTH=1

Nothing in the (disconnected) console, but a bit of information on the command line:

<--- Last few GCs --->
arking 47 ms) (average mu = 0.915, current mu = 0.978) finalize incremental[70:0x560853e63640]     1612 ms: Mark-sweep 1721.3 (1725.4) -> 1574.0 (1578.1) MB, 5.0 / 0.0 ms  (+ 0.8 ms in 4 steps since start of marking, biggest step 0.6 ms, walltime since start of marking 1216 ms) (average mu = 0.993, current mu = 0.996) allocation[70:0x560853e63640]     1706 ms: Mark-sweep 1574.0 (1578.1) -> 577.5 (581.6) MB, 94.7 / 0.0 ms  (average mu = 0.876, current mu = 0.000) allocation failure GC in old space requested


<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x56084ddc86ee]
Security context: 0x06953a021079 <String[21]: http://localhost:8001>
    1: write [0x3225aa7a43a1] [http://localhost:8001/testlarge.js:~2400] [pc=0x30bbd6ae1b21](this=0x3225aa7a1d99 <Object map = 0x273c6e0b3851>,0x07eb43631a91 <JSObject>,0x3225aa7a1d51 <Int8Array map = 0x273c6e0a4f81>,6640,1048576,112197632,0x2ffb7b2025b1 <undefined>)
    2: write [0x3225aa7a1de1] [http://localhost:8001/testlarge.js:4...
@kripken

This comment has been minimized.

Copy link
Member

kripken commented Dec 10, 2018

Best to file an issue on the chromium tracker directly, I think: https://bugs.chromium.org/p/chromium/issues/list (and please link to it here if you do).

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Dec 11, 2018

@juj

This comment has been minimized.

Copy link
Collaborator

juj commented Dec 11, 2018

Chromium has had a longstanding bug where allocating a large ArrayBuffer would not throw an exception if it ran out of memory, but instead they would make the whole page AwwSnap - this might be something similar to that. https://bugs.chromium.org/p/chromium/issues/detail?id=42342 , https://bugs.chromium.org/p/chromium/issues/detail?id=257018 , https://bugs.chromium.org/p/chromium/issues/detail?id=536816 are some reports. They have stated it to be fixed various times in the past, and have seen commits go in that do fixes, but I presume the issue is wider and dependent on code flow/context, making the problem appear in multiple different scenarios, which is why the issue keeps reappearing. If you have good deterministic test case build to attach to the bug report, that may be useful.

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Dec 11, 2018

Thanks for the input.
I did submit the 100%-crash code I posted above, but looking at the bugs you mentioned they seem to already have that kind of test case?

@towca

This comment has been minimized.

Copy link

towca commented Jan 18, 2019

@Beuc, have you heard anything on the issue, or managed to deal with it somehow?

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Jan 18, 2019

No :/ You could ping the chromium bug item https://bugs.chromium.org/p/chromium/issues/detail?id=913861 .

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Jan 18, 2019

@sbc100 upstream's feedback is not optimistic ("probably not something we're likely to be able to support") - can you check if there's a way forward?

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Jan 22, 2019

@kripken wrt to your answer at chromium, maybe my question is naive, but is there a way to store efficiently (1 byte -> 1 byte) the file in memfs? Either in the memfs internals (like malloc), or at the application level by somehow pre-allocating the file?

(or I could ask users to convert their .zip to a .data+.js - but since I plan to implement compression at the application level due to web hosts limitation, I'd probably get the same issue?)

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Jan 22, 2019

@Beuc well, that's what happens in the typed array case, 1 byte is 1 byte there. We only use normal JS arrays because they can append, so if you do many appends it can be faster. But in v8 such arrays take 8x the space, turns out, so it really is not a good idea for large arrays.

Options here may include:

  • Just removing the MEMFS_APPEND_TO_TYPED_ARRAYS and always use typed arrays, even when appending. This may be slow in some cases, but won't fail pathologically in others.
  • Keep the option but switch to it by default, so users can change to the old behavior if desired.

@juj, any objections to removing that option (first of the 2 items here)? I think you may have cared about it.

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Jan 22, 2019

I see we have a pretty careful logic for the typed array case, we don't allocate for every single append, and we don't double the size each time we allocate. So it may really be just good enough to use that path, and disallow JS array appending.

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Jan 22, 2019

This may be slow in some cases

That's what I'd like to avoid, since decompressing the game data on start-up is slow enough already :/
I'll try and time MEMFS_APPEND_TO_TYPED_ARRAYS when I'm done with my other pending issues.

Since RAM is tight (esp. on mobile devices) using it as default sounds good to me.

My question was also: to maintain good perfs, is there a way to use typed array efficiently for generated files (from the C API), or is it something that can only be done from JavaScript (e.g. from the file packager's runtime)? But if things are efficient enough by default maybe we don't care.

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Jan 22, 2019

Not sure why there should be a difference between the C API and JS APIs - maybe I don't know which APIs you mean? Do you mean the C stdio API and the JS FS APIs? (both should end up using the code in library_memfs.js)

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Jan 23, 2019

To put it differently, is there a way to rewrite this bug's test case so that (efficient) typed arrays are always used?

@juj

This comment has been minimized.

Copy link
Collaborator

juj commented Jan 23, 2019

@juj, any objections to removing that option (first of the 2 items here)? I think you may have cared about it.

Sorry, can you rephrase what is being proposed here? Is the intent to make -s MEMFS_APPEND_TO_TYPED_ARRAYS=1 the default build option, and remove support for -s MEMFS_APPEND_TO_TYPED_ARRAYS=0 case? That plan does sound ok for me.

However before going down that path, has it been confirmed that using the linker flag -s MEMFS_APPEND_TO_TYPED_ARRAYS=1 avoids the Chrome crash issue? Scanning the comment history above, I am not sure if that was yet tested?

@Beuc

This comment has been minimized.

Copy link
Contributor Author

Beuc commented Jan 23, 2019

I checked the test case and my initial program (.zip extraction).
Using MEMFS_APPEND_TO_TYPED_ARRAYS=1 not only stops crashing under Chromium, but is also faster.

Updated testcase with some timing:

#include <stdio.h>
#include <stdlib.h>

#define BUFSIZE 1024*1024
#include <sys/time.h>
  

int main(void) {
  FILE* f = fopen("/archive.rpa", "w");
  unsigned char buf[BUFSIZE] = "";
  for (int i = 0; i < BUFSIZE; i++) {
    buf[i] = rand() % 256;
  }

  struct timeval tv1, tv2;
  gettimeofday(&tv1, NULL);

  for (int i = 0; i < 150; i++) {  // fails at 108 for me
    fwrite(buf, BUFSIZE, 1, f);
  }

  gettimeofday(&tv2, NULL);
  printf("Wrote file in %fs.\n",
	 (tv2.tv_sec - tv1.tv_sec) + (tv2.tv_usec - tv1.tv_usec) / 1000000.0);
}

Typically 10x faster for the test case, and ~10-40% faster for the .zip extraction.

I suppose writing 8x less in memory has its benefits, even with some reallocation :)

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Jan 23, 2019

Cool, thanks @Beuc!

@juj yes, the plan as you said it, remove the option, and always use typed arrays. I opened #7918 for this.

@kripken kripken closed this in bdb8e87 Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment