Skip to content
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

JS version #65

Closed
mstange opened this issue Feb 28, 2017 · 13 comments
Closed

JS version #65

mstange opened this issue Feb 28, 2017 · 13 comments

Comments

@mstange
Copy link
Contributor

mstange commented Feb 28, 2017

Have you tried compiling this to asm.js / wasm?

I wonder if the result would be smaller than http://searchfox.org/mozilla-central/source/devtools/client/shared/demangle.js which is 277 KB. If so, I'd like to use it for firefox-devtools/profiler#167.

@fitzgen
Copy link
Member

fitzgen commented Feb 28, 2017

That would be super cool! I have not tried compiling to asm.js or wasm. Perhaps you can give it a go and report back?

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

I'll give it a try.

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

It works!

It's about twice as large as the other one (516 KB vs 277 KB), and it fails a few tests from https://github.com/kripken/cxx_demangle/blob/master/test/test.js :

__Znwj
 returns:  operatornew(unsigned int)
 expected: operator new(unsigned int)

__ZL12abcdabcdabcdi
 <fails>
 expected: abcdabcdabcd(int)

__Z5multiwahtjmxyz
 returns:  multi(wchar_t, signed char, unsigned char, unsigned short, unsigned int, unsigned long, long long, unsigned long long, ellipsis)
 expected: multi(wchar_t, signed char, unsigned char, unsigned short, unsigned int, unsigned long, long long, unsigned long long, ...)

__ZN21FWakaGLXFleeflsMarfooC2EjjjPKvbjj
 returns:  FWakaGLXFleeflsMarfoo::base object constructor(unsigned int, unsigned int, unsigned int, void const*, bool, unsigned int, unsigned int)
 expected: FWakaGLXFleeflsMarfoo::FWakaGLXFleeflsMarfoo(unsigned int, unsigned int, unsigned int, void const*, bool, unsigned int, unsigned int)

__ZN5wakaw2Cm10RasterBaseINS_6watwat9PolocatorEE8merbine1INS4_2OREEEvPKjj
 returns:  void wakaw::Cm::RasterBase<wakaw::watwat::Polocator>::merbine1<wakaw::watwat::Polocator::OR>(unsigned int const*, unsigned int)
 expected: void wakaw::Cm::RasterBase<wakaw::watwat::Polocator>::merbine1<wakaw::Cm::RasterBase<wakaw::watwat::Polocator>::OR>(unsigned int const*, unsigned int)

The timings in that test also report a slight regression (~250ms instead of ~230ms for 1000 invocations), but there was a lot of variance, and part of the regression might be caused by the extra string copy that my extern C wrapper function does in order to get a null-terminated C string.

I'm going to clean up my experiment and push it to github.

@fitzgen
Copy link
Member

fitzgen commented Feb 28, 2017

Is there anything I can add to the Travis CI builds to make sure that asm.js/wasm compilation doesn't accidentally break in the future?

and it fails a few tests from

I'll look into these once I get all the libiberty tests passing (which I suspect might fix these failures as well).

The timings in that test also report a slight regression (~250ms instead of ~230ms for 1000 invocations), but there was a lot of variance,

FWIW, I haven't looked at performance at all yet.

and part of the regression might be caused by the extra string copy that my extern C wrapper function does in order to get a null-terminated C string.

FWIW, cpp_demangle doesn't need to take ownership of the string. See BorrowedSymbol for the non-owning version. That won't matter if the symbol isn't already in an array buffer, however.

For the owned and borowed versions, it doesn't need a null terminator.

@fitzgen
Copy link
Member

fitzgen commented Feb 28, 2017

It's about twice as large as the other one (516 KB vs 277 KB)

I wonder what the code size profiling tools are like for asm.js/wasm (let alone Rust integration) ... Any idea?

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

Not sure about CI. Installing the emscripten SDK every time the test runs might be a bit much, I think it even compiled parts of llvm for me... If you want to test the JS compiled result, you'll also need to run all the stuff from the cxx_demangle repo and its tests.

Passing the string to cpp_demangle doesn't copy, I think; it's when I return the result that I need to do the copy. I call CString::new (because that's apparently how you're supposed to do this), and if I interpret the documentation correctly, that one copies.

I don't know anything about emscripten code size profiling tools.

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

The extra copy is definitely avoidable. We don't have to return a const char* there, we could also just fill a user-provided buffer and return a length. That would probably be better. It would also make the job easier for the emscripten-supplied function that converts a C string into a JS string. That goes through UTF8ArrayToString which searches for the null byte again in order to know the length of the string, and then (for strings longer than 25 bytes) calls u8Array.subarray which also makes a copy.

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

That was wrong; u8Array.subarray doesn't make a copy. (But it allocates a JS object, i.e. garbage, which UTF8ArrayToString wants to avoid.)

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

I got it down to 455 KB by following the instructions at https://kripken.github.io/emscripten-site/docs/optimizing/Optimizing-Code.html#code-size and I also eliminated the copy.

@fitzgen
Copy link
Member

fitzgen commented Feb 28, 2017

I got it down to 455 KB by following the instructions at https://kripken.github.io/emscripten-site/docs/optimizing/Optimizing-Code.html#code-size and I also eliminated the copy.

Nice! Did that improve performance?

@mstange
Copy link
Contributor Author

mstange commented Feb 28, 2017

Not noticeably so. I haven't run the perf tests for a high enough number of iterations to have enough certainty.

@mstange
Copy link
Contributor Author

mstange commented May 20, 2020

Closing - this doesn't seem to be necessary. wasm-pack and wasm_bindgen make it easy enough for anybody to write their own wasm / JS wrapper.
For example, I have one at https://www.npmjs.com/package/gecko-profiler-demangle .

@mstange mstange closed this as completed May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants