Skip to content

Performance drop since 2.0.2 release #13796

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

Open
jpharvey opened this issue Mar 30, 2021 · 7 comments
Open

Performance drop since 2.0.2 release #13796

jpharvey opened this issue Mar 30, 2021 · 7 comments

Comments

@jpharvey
Copy link

We recently tried to move to 2.0.15 and noticed a 10-20% increase in run time of our application, this also showed up in tests that are allocating memory. We traced this back to starting at 2.0.2.
The attached programme which i compile with
call em++ -s DISABLE_EXCEPTION_CATCHING=0 -s INITIAL_MEMORY=1024MB -s ASSERTIONS=0 -s DEMANGLE_SUPPORT=0 -O3 -DNDEBUG=1 test.cpp -o test.js
and the run with
node test.js
shows time for 2.0.1 to do the malloc's
Time difference = 5447635 msecs
and for 2.0.2
Time difference = 8133452 msecs

I have looked through the commits in emscripten between the 2 releases and don't understand which of them is causing this.
It doesn't matter if we turn exception handling on or off.

@kripken
Copy link
Member

kripken commented Mar 30, 2021

I recommend bisecting this,

https://emscripten.org/docs/contributing/developers_guide.html#bisecting

That should quickly find a much smaller range.

@jpharvey
Copy link
Author

Sorry for the slow response but i now understand what caused the change in performance.
In emscripten-release it happens at git sha 045dcd26b4b51b04d788f5ad2bc332c9890c5da9 - emscripten from b1a7889 to c4ac7b7 (2 revisions)

In emscripten this was #12010 and #11979
These just affect the js wrapper as far as i can see and i have taken the js wrapper that comes from the code built with the previous emsripten release sha baf0210cff89449053a305934babb135a5cccde3 and copied it alongside the wasm file from the later version and the performance returns.
In our application while we are loading the data the times are increasing by between 35% & 65% depending on how much i/o is involved.
The tests i have run just now are using Chrome

@kripken
Copy link
Member

kripken commented May 7, 2021

I think the issue here might be the use of wasmTable.get().

@juj I think we had an agreed direction to cache the values of the table (but I can't seem to find the issue now) and I think I remember you had a PR for that? If not I can look into adding a cache for this.

@juj
Copy link
Collaborator

juj commented May 9, 2021

@juj I think we had an agreed direction to cache the values of the table (but I can't seem to find the issue now) and I think I remember you had a PR for that?

The PR is #13844 , looks like it has been ready for review and landing, but has gone stale.

@kripken
Copy link
Member

kripken commented May 10, 2021

Thanks @juj, I commented there now. Looks close to ready to land to me.

@jpharvey
Copy link
Author

jpharvey commented May 10, 2021

@kripken @juj thanks for looking a this. I just tested 2.020 with the changes from #13844 that still leaves all the invoke_ii etc. functions that call wasmTable.get directly and so this doesnt fix our performance problem. Replacing those with wbind seems to fix it though. I just hacked that into the js file to test.

@jpharvey
Copy link
Author

@juj Is there anything i can do to help get that PR approved/merged. The additional change that is needed for us to get our performance back is to replace the wasmTable.get in make_dynCall with wbind in tools/shared.py

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

3 participants