-
Notifications
You must be signed in to change notification settings - Fork 39
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
vm: restore the VM profiler #912
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary ======= Fix the compiler crashing when enabling the VM profiler (`--profilevm`) and fully restore the latter's original functionality. In addition, the profiler's overhead is significantly reduced. Details ======= The VM profiler stopped working when the stack-frames moved from being `ref` based to being `seq` based (fb03691). This is due to the profiler expecting the frame index at the start of a measurement (`enter`) being valid at the end of a measurement (`leave`), but this is not true when executing, for example, a `Ret` instruction, which led to an out-of-bounds error. As a simple and low impact fix, instead of remembering the index, `leave` now takes the list of current stack frames (i.e., the stack) as input, treating the last item as the current frame. A minor side-effect of this is that a `Ret` instruction now counts for the caller, while a call instruction counts for the callee. There was also a bug in the rendering logic: `min` was used instead of `max`, meaning that the entry limiting didn't work. In addition, the documentation of the `vmprofiler` module is improved and the implementation is refactored: * the `data` table now uses a `PSym` as the key. The intention of the table was always to associate data with a procedure, but `TLineInfo` and later `SourceLinePosition` only approximated this. Using a `PSym` also makes it possible to render more information (such as the procedure's name) in the future * whether profiling is enabled is now a property of the `Profiler` instance, removing the dependency on a `ConfigRef`. This also slightly reduces the overhead of `enter`/`leave` * the `leaveImpl` procedure is improved. The time difference is only computed once, and instead of temporarily assigning each frame to a local (which incurred a very costly full copy), only `prc` is assigned to a local With access to the symbol, rendering now also renders the full file, line, and column of the profiled procedures again (instead of only the containing file). Finally, an integration test (`tprofiler.nim`) and a basic unit test (`tvmprofiler.nim`) are added in order to prevent future regressions.
zerbina
added
bug
Something isn't working
refactor
Implementation refactor
compiler/vm
Embedded virtual machine
labels
Sep 16, 2023
saem
approved these changes
Sep 17, 2023
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'm looking forward to folks using this to debug various issues
/merge |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix the compiler crashing when enabling the VM profiler (
--profilevm
)and fully restore the latter's original functionality. In addition, the
profiler's overhead is significantly reduced.
Details
The VM profiler stopped working when the stack-frames moved from being
ref
based to beingseq
based (fb03691).This is due to the profiler expecting the frame index specified at the
start of a measurement (
enter
) still being valid at the end of ameasurement (
leave
), but this is not true when executing, for example,a
Ret
instruction, which led to an out-of-bounds error.As a simple and low impact fix, instead of remembering the index,
leave
now takes the list of current stack frames (i.e., the stack) asinput, treating the last item as the current frame. A minor side-effect
of this is that a
Ret
instruction now counts for the caller, while acall instruction counts for the callee.
There was also a bug in the rendering logic:
min
was used instead ofmax
, meaning that the entry limiting didn't work.In addition, the documentation of the
vmprofiler
module is improvedand the implementation is refactored:
data
table now uses aPSym
as the key. The intention of thetable was always to associate data with a procedure, but
TLineInfo
and later
SourceLinePosition
only approximated this. Using aPSym
also makes it possible to render more information (such as the
procedure's name) in the future
Profiler
instance, removing the dependency on a
ConfigRef
. This also slightlyreduces the overhead of
enter
/leave
leaveImpl
procedure is improved. The time difference is onlycomputed once, and instead of temporarily assigning each frame to a
local (which incurred a very costly full copy), only
prc
is assignedto a local
With access to the symbol, rendering now also renders the full file,
line, and column of the profiled procedures again (instead of only the
containing file).
Finally, an integration test (
tprofiler.nim
) and a basic unit test(
tvmprofiler.nim
) are added in order to prevent future regressions.