Skip to content
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 1 commit into from
Sep 17, 2023

Commits on Sep 16, 2023

  1. vm: restore the VM profiler

    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 committed Sep 16, 2023
    Configuration menu
    Copy the full SHA
    47a633b View commit details
    Browse the repository at this point in the history