windows: emit ETW events with v8 stack trace information #4219
Conversation
| static uv_async_t dispatch_etw_events_change_async; | ||
|
|
||
| struct v8tags { | ||
| const char *prefix; |
There was a problem hiding this comment.
Style police: 2 spaces, not 4, and const char* instead of const char *
There was a problem hiding this comment.
Oh, another thing - since this data structure will be read often, do it like this:
struct v8tags {
char prefix[32 - sizeof(size_t)];
size_t prelen;
};
Avoids a level of indirection and makes it fit in a single cache line. Presumably prelen doesn't have to be a size_t because it's never going to be very large.
Added two new ETW writer macros that accept an existing string length so as to not make a redundant strlen or wcslen call. I'm using the new version of the macro in the one place that I could find where the string length is already known in context. Other reductions are probably possible here, but would take more plumbing (and changes in dtrace code as well).
| INLINE void NODE_V8SYMBOL_MOVE(const void* addr1, const void* addr2); | ||
| INLINE void NODE_V8SYMBOL_RESET(); | ||
| INLINE void NODE_V8SYMBOL_ADD(LPCSTR symbol, | ||
| int symbol_len, |
Incorporate feedback from @piscisaureus and @bnoordhuis from #4219
|
Okay--I believe my latest commit incorporates 100% of the review feedback. I've built release and debug on 64-bit Windows. I've tested that we're actually getting the ETW events that we claim this patch introduces and did some debuggering to make sure our failure paths (NULL, Invalid) do what we expect even though we never expect them. Do we have a pre-commit checklist that includes anything I'm neglecting here? /cc @piscisaureus, @bnoordhuis |
Not really. But generally we ask ourselves:
|
|
LGTM, and landed in 66f64ae. Thanks @HenryRawas and @sblom. |
Latest version of @HenryRawas's V8 symbols via ETW code. Previous version reviewed by @piscisaureus and @bnoordhuis at https://github.com/MSOpenTech/node/commit/1a59ffbd31cf06f6c7e0f5c807366ec733c8e153. I'm on deck to address any feedback/issues with this change.