-
-
Notifications
You must be signed in to change notification settings - Fork 416
made windows stack traces 10 times faster #638
Conversation
return result; | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we still need the old stack trace code then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because:
- RtlCaptureStackBackTrace does not work when given a exception context
- RtlCaptureStackBackTrace does not use the debug information to resolve stack frames (only works when all functions have frame pointers)
- RtlCaptureStackBackTrace has a limit of 63 stack frames it can walk
So for these three cases the old code is still needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to add this explanation as a comment in the code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's briefly mentioned in the sources, but I agree a more thorough explanation in the sources or in the commit message would be nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to add this explanation as a comment in the code!
So whats the preffered action now? Should a create a new pull request or can I commit into this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to improve the comment please open a new pull request and reference this one.
} | ||
else | ||
{ | ||
auto result = new ulong[backtraceLength]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need to get rid of any GC allocations in this module, but that can remain a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know thats an issue. Since when is removing allocations from druntime a priority?
I actually have a non allocating version but rewrote it, because its harder to use and somewhat ugly.
https://github.com/Ingrater/druntime/blob/merge63/src/core/sys/windows/stacktrace.d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since always, it complicates the initialization order and most GC allocations are not needed because internally druntime mostly needs allocations to track state, i.e. uses static references. It's a little different for use faced core functions. The particular issue in this module is that it's also used to throw Errors so the runtime is in an undefined state and a GC allocation could for example deadlock.
made windows stack traces 10 times faster
Can't we specialize this and allow a bigger buffer for >XP ? |
We most likely could somehow. How often do you have stack depths > 63? |
In some projects I'm forced to catch some exceptions when I'm in a C callback, and then re-throw them once I'm back in D
It's still not over the |
Well its not like it will stop working if there are more then 63 stackframes. It will just be slower. If this is a issue for you I can dig for a way to detect Windows XP and add a condition to use a bigger buffer if running on > XP |
It's not an issue, don't worry about it. |
This change makes the tracing part of stack tracing on windows about 10 times faster. This will especially impact throwing exceptions, which will be a lot faster now.