-O2 fails on Sundown (-O1 works) #491

Closed
bootstraponline opened this Issue Jun 26, 2012 · 25 comments

Comments

Projects
None yet
2 participants

Running bash to_js.sh on my sundown repo creates o1, o2, and o3 versions of sundown.js from sundown.c.

o1 works correctly and o2 fails when invoked multiple times.

Error from -O2 is:

Uncaught TypeError: Property '1702129525' of object 0,0,function Fe(b){Ge(b|0)},0,function yc(b){var e=s;s+=124;var

Owner

kripken commented Jun 26, 2012

I built it without error. Or is that a run error when the code is executed? If so, what parameter should I use? (without params it seems to do nothing and halt without error)

Yes it builds correctly, however it errors when run multiple times. I setup an example which calls the function multiple times. The first thousand invocations are usually error free. The o1 build does not trigger the error.

src/settings.js must be edited to include _str_to_html in exported functions.

var EXPORTED_FUNCTIONS = ['_main', '_malloc', '_free', '_str_to_html'];

This is the test I'm using.

Owner

kripken commented Jun 26, 2012

Ok, what is the simplest way for me to reproduce the problem? Those links have me confused. I have sundown_o0.js and sundown_o2.js, how can i see a case where one works and one doesn't?

Sorry for the confusion. I've tried to make it simple by already creating the html test case so you just have to open it in a browser.

Download this zip (-O1 works) and open /public/index.html in Chrome.

Follow the same steps with this zip (-O2 failure) for the failure case.

Owner

kripken commented Jun 27, 2012

Sorry, I might have misunderstood before. But the zips made it very easy to test now.

So, this works fine in Firefox but breaks in Chrome. Looks like this is a Chrome bug then. If you can please file a bug on them and link to it from here so we can follow it.

Owner

kripken commented Jun 27, 2012

Btw, what is being benchmarked here? It says emscript vs. pagedown, I'm not sure what those are. (well, specifically not sure what the second is, and what is compiled in the first ;)

Thanks for looking into this issue. I'll file a bug report tomorrow on the Chrome tracker and will link it here.

Sundown is GitHub's markdown software. I'm adding live preview support to Gollum and wanted to use sundown instead of pagedown. Pagedown is what StackOverflow uses to display their markdown.

There were two concerns raised about the emscripten compiled version of sundown.

  • Is it slower than pagedown?
  • Is it too large?

The emscripten version turns out to be significantly faster. With -O2 it's also an acceptable size, the only blocking issue is that the -O2 version errors on Chrome.

Owner

kripken commented Jun 28, 2012

Thanks for the info, very interesting!

I updated the test case to run 10k iterations. The bug is not Chrome specific, both Firefox and Chrome crash on the same iteration.

Owner

kripken commented Jun 28, 2012

Was Chrome running more iterations before? (It was slower so the opposite seems more likely)

Anyhow, can you make two builds for debugging, one with -O2 --minify 0 (unminified) and one with -O2 --closure 0 unclosured?

Owner

kripken commented Jun 28, 2012

And if you can, also a variation of each of those with -s ASSERTIONS=1. Might be a stack overflow or such.

I don't know how many iterations were running before because I was using benchmark.js. I decided to create a simplified test case so it was obvious what the code was doing.

Here are the four new versions.

emcc -O2 --minify 0 sundown.bc -o sundown_o2_m0.js
emcc -O2 --closure 0 sundown.bc -o sundown_o2_c0.js
emcc -O2 --minify 0 -s ASSERTIONS=1 sundown.bc -o sundown_o2_m0_a1.js
emcc -O2 --closure 0 -s ASSERTIONS=1 sundown.bc -o sundown_o2_c0_a1.js
Assertion failed: Ran out of stack:
Error
    at abort (file://bootstraponline-livepreview_benchmark_o2-e8a491b/public/js/sundown_o2.js:399:32)
    at assert (file://bootstraponline-livepreview_benchmark_o2-e8a491b/public/js/sundown_o2.js:406:5)
    at Array.stackAlloc [as 1] (file://bootstraponline-livepreview_benchmark_o2-e8a491b/public/js/sundown_o2.js:354:115)
    at allocate (file://bootstraponline-livepreview_benchmark_o2-e8a491b/public/js/sundown_o2.js:557:115)
    at file://bootstraponline-livepreview_benchmark_o2-e8a491b/public/js/test.js:60:38 sundown_o2.js:70
Assertion: Assertion failed: Ran out of stack test.js:62
crashed on iteration #9781 test.js:63
Uncaught Assertion: Assertion failed: Ran out of stack 

Does this mean there's a bug in my C code?
Or do I have to release something after calling allocate in JavaScript?

It even errors now when there are no optimizations.
emcc -s ASSERTIONS=1 sundown.bc -o sundown_a1.js

Owner

kripken commented Jun 29, 2012

Ok, good to know this is a stack overflow, that kind of thing is exactly why we have the ASSERTIONS setting (note that assertions are on in -O0 by default, so this would have been caught if the same # of iterations were done in all cases, I believe). This should not be too hard to debug, in fact it looks like in the JS code the issue is no free after allocate. You are allocating on the stack there, but not restoring the stack position.

One option is to do x = Runtime.stackSave() and later Runtime.stackRestore(x). Another is simply to allocate that string once outside of the loop (it doesn't change), as stack allocation takes zero time anyhow so it doesn't affect the benchmark.

In the production version of the code the string is dynamic.

I tried stackSave and stackRestore, however Runtime is undefined. Runtime also isn't defined as a module. What's the proper way to obtain a reference to Runtime?

    var stack = 0; // outside of the loop
    stack = Runtime.stackSave(); // Runtime is undefined
    Pointer_stringify( _str_to_html( allocate( intArrayFromString( text ), 'i8', ALLOC_STACK ) ) );
    Runtime.stackRestore(stack);
Owner

kripken commented Jun 29, 2012

Runtime might not be exported in a closure-compiled build. But actually, if the string is dynamically allocated normally, then just use malloc. Or more specifically, allocate with Module.allocate(.., Module.ALLOC_NORMAL) and then free with Module._free (that's the free stdlibc function).

Normal malloc/free makes more sense for the general case because the stack might not be big enough for a single large enough string.

Owner

kripken commented Jun 29, 2012

Or it might be nicer to do x = Module._malloc(..); Module.writeStringToMemory(js_str, x); [.. use it ..] Module._free(..)

This works.

var pointer = Module._malloc( text.length ) ;
Module.writeStringToMemory( text, pointer );
console.log(  Pointer_stringify( _str_to_html( pointer ) ) );
Module._free( pointer );

It tanked performance though. Is there a more performant way of dynamically allocating memory as the string increases in size? In the case of live preview, it's recalculated every time a character is added or deleted.

pagedown length: 852 test.js:76
ems length: 893 test.js:77
emscript x 377 ops/sec ±177.99% (96 runs sampled) test.js:89
pagedown x 515 ops/sec ±2.12% (88 runs sampled) test.js:89
Fastest is pagedown 
Owner

kripken commented Jun 29, 2012

It sounds like you should allocate a large buffer once and never free it. Just check that it is big enough for the current string and replace it with a bigger one if necessary. Then you will have basically no allocations.

I found realloc which seems useful. Thanks for your help!

Owner

kripken commented Jun 29, 2012

realloc will still be quite slow. You can just allocate one buffer and never free it as I said before. You should not need to do allocations in this situation since the buffer is only used as temporary storage.

This is how I'm using realloc. It's still just allocating one buffer and never freeing it. I think without realloc, if the text size increases then the program will break. In live preview the text can expand to arbitrary sizes.

Is there a better way? This is the new benchmark code.

// Init.
var allocSize = text.length + 1024;
var pointer = malloc( allocSize ) ;

for ( var i=0; i < MAX; i++ ) {
  if ( text.length > allocSize ) {
    allocSize = text.length << 1; // double
    pointer = realloc( pointer, allocSize );
  }
  writeStringToMemory( text, pointer );
  Pointer_stringify( _str_to_html( pointer ) );
}

free( pointer );
Owner

kripken commented Jun 29, 2012

That looks great. The way I was thinking of before was not as good actually.

Thanks for your help. I benchmarked the new code and emscripten is significantly faster again.

emscript x 5,243 ops/sec ±1.26% (9 runs sampled) test.js:44
pagedown x 527 ops/sec ±2.34% (9 runs sampled) test.js:44
Fastest is emscript 
Owner

kripken commented Jun 29, 2012

Very impressive! I've started to follow the issue on gollum too

https://github.com/github/gollum/issues/404#issuecomment-6670367

Looks like you proved the opposite of what they were expecting, very cool.

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