[memoryprofiler] Show allocations from threads#26175
[memoryprofiler] Show allocations from threads#26175sbc100 merged 6 commits intoemscripten-core:mainfrom
Conversation
matzf
force-pushed
the
feature/memoryprofiler-pthread
branch
2 times, most recently
from
95777be to
ba2463e
Compare
|
@juj, friendly ping, you were assigned as reviewer for this PR. Please let me know if you need anything from me to move this forward. |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Sorry I forgot to send these comments from earlier.
src/lib/libtrace.js
Outdated
| emscripten_trace_record_free: (address) => { | ||
| Module['onFree']?.(address); | ||
| #if MEMORYPROFILER | ||
| Module['onFree']?.(address) || postMessage({cmd: 'callHandler', handler: 'onFree', args: [address]}); |
There was a problem hiding this comment.
Seems like this could be abstracted into a helper function.
src/memoryprofiler.js
Outdated
| var loc = new Error().stack.toString(); | ||
| if(loc == undefined) { | ||
| loc = new Error().stack.toString(); | ||
| } |
There was a problem hiding this comment.
How about we just expect loc to be defined by the caller? i.e. can you instead assert(loc) here, and then all the callsites can be in charge of calling new Error().stack.toString()
sbc100
changed the title
|
LGTM. One note of concern I have though is that we still have an unfixed design problem/regression from #19683 (comment) , which revolves around the In that discussion I had made the recommendation to remove the Still, this is "only" for debug code, and I don't have a good solution for the above issue, so maybe that is not a good thing to block on for this. |
matzf
force-pushed
the
feature/memoryprofiler-pthread
branch
from
4b806ed to
d1eb5c7
Compare
matzf
force-pushed
the
feature/memoryprofiler-pthread
branch
from
d1eb5c7 to
72ddb60
Compare
|
Thanks for your reviews! I found that the existing automatic-ish proxying of the Module methods in libpthread can be used to forward the trace/memoryprofiler hooks to the main thread. With this, I don't need the postMessage calls scattered around. |
|
I can update the codesize expectation for this PR |
This allows for the complete removal of `libmemoryprofiler.js` since the memory profiler can now just use the same libtrace-based hooks for all events. I noticed this while reviewing #26175
3 participants
Support multi-threaded applications in memoryprofiler.
Fixes #18107.