EmscriptenApplication: use _malloc instead of internal allocate #483
Conversation
There was a problem hiding this comment.
This is great, thanks!
There's one more actually two more occurences of allocate() in Corrade's Utility::Arguments (and here), could I ask you to update that as well? Thanks 🙏 To test, it should be enough to build the UtilityArgumentTest (with BUILD_TESTS enabled in CMake) and run node ./Release/bin/UtilityArgumentsTest.js (possibly with -s ASSERTIONS=1 enabled).
| "-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=0") | ||
| "-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=0" | ||
| # Enable memory runtime checks | ||
| "-s ASSERTIONS=2 -s SAFE_HEAP=1") |
There was a problem hiding this comment.
Are there any downsides of enabling assertions? I suppose it will increase the resulting JS file size quite a bit -- I occasionally use these tests to check changes in build sizes, so I guess I'll have to remember to temporarily comment this out when doing the comparisons.
I was thinking about enabling this only for Debug builds, but I personally almost never build for Emscripten in Debug, so that wouldn't help in catching new breakages.
There was a problem hiding this comment.
You could enable it for the multiple canvas test app only, so it's tested but you still have the normal application to test with?
There was a problem hiding this comment.
Eh, let's keep it there :) If it gets in the way of something, I'll remove it, but for now better safe than sorry.
| const memory = _malloc(bytes); | ||
| stringToUTF8(element.id, memory, bytes); | ||
| return memory; | ||
| } |
There was a problem hiding this comment.
Did you see any significant change in build output size with this change present? I hope malloc doesn't drag along tens of kilobytes of JS code that wouldn't be needed with plain allocate :)
There was a problem hiding this comment.
136kb before, 137kb after, 191kb with ASSERTIONS and SAFE_HEAP
Codecov Report
@@ Coverage Diff @@
## master #483 +/- ##
=======================================
Coverage 77.12% 77.12%
=======================================
Files 415 415
Lines 25502 25502
=======================================
Hits 19669 19669
Misses 5833 5833 Continue to review full report at Codecov.
|
|
I finally managed to get at least some CI jobs running and merged as f940358. Thank you! |
Fix for
allocatehaving a different function signature starting with Emscripten 2.0.5.I think this is cleaner than
#ifdef-ing around__EMSCRIPTEN_major/minor/tiny__as that internal function could change again in the future, and duplicatingEM_ASMblocks means even more code and maintenance.I added
ASSERTIONSandSAFE_HEAPto the test application compile flags so similar issues get caught in the future.cc: @Squareys