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

enable VM tracing in user code via {.define(nimVmTrace).} #18244

Merged
merged 3 commits into from
Jun 24, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jun 12, 2021

features

This PR allows user code to enable/disable VM tracing in a code section via {.define(nimVmTrace).} and {.undef(nimVmTrace).}.

Unlike -d:nimVMDebug, this doesn't require re-compiling nim and allows user code to control what sections of code should be traced ( -d:nimVMDebug by contrast generates lots of noise and cannot be controlled by user code). The check whether to trace doesn't slow down VM thanks to a caching mechanism.

example

proc main1() =
  var a = 1
  a.inc

proc main2() =
  var a = 1
  a.inc
  a+=2

{.define(nimVmTrace).}
static: main1()
{.undef(nimVmTrace).}
static: main2()
nim r --filenames:canonical -f main

tests/nim/all/t12391.nim(15, 9) [opcLdConst] static: main1()
tests/nim/all/t12391.nim(15, 14) [opcIndCall] static: main1()
tests/nim/all/t12391.nim(6, 11) [opcLdImmInt]   var a = 1
tests/nim/all/t12391.nim(7, 4) [opcAddImmInt]   a.inc
tests/nim/all/t12391.nim(6, 3) [opcRet]   var a = 1
tests/nim/all/t12391.nim(6, 3) [opcJmp]   var a = 1
tests/nim/all/t12391.nim(6, 3) [opcEof]   var a = 1

future work

  • if this doesn't result in a performance penalty, consider also moving the when traceCode: into a similar flag that can be set similarly, without recompiling nim
  • likewise for VM profiling
  • likewise for VM counters (counting the number of times a certain event happens)

compiler/vm.nim Outdated Show resolved Hide resolved
compiler/options.nim Outdated Show resolved Hide resolved
compiler/options.nim Outdated Show resolved Hide resolved
compiler/pragmas.nim Outdated Show resolved Hide resolved
compiler/pragmas.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Jun 14, 2021

But please notice that even after my remarks, the mechanism is most alien. We don't enable features via .define/.undef pragmas in a block scope manner, there are much better mechanisms available.

Like:

static:
  trace: 
     main1()

@timotheecour
Copy link
Member Author

timotheecour commented Jun 14, 2021

the only reason I used {define}/{undef} was to avoid adding a magic or vmops, but I agree that a magic or vmops is cleaner (and a bit more efficient) here; i'll update this PR with a magic or vmops (TBD), how about adding that API in std/vmutils which can grow more APIs later (eg to control VM profiling via API calls to give more control compared to current --profileVM)

(the existing lib/std/private/vmutils.nim is unrelated and can be renamed for clarity)

@timotheecour timotheecour removed the Ready For Review (please take another look): ready for next review round label Jun 14, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Jun 21, 2021

PTAL, now using a vmops which is more efficient (single bool flag lookup) and flexible (can be set/unset during the course of VM evaluation, unlike define/undef)

see test in PR (nimout is truncated but you can see the full listing by running locally)

@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jun 21, 2021
changelog.md Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Jun 21, 2021

The implementation is ok but don't document it and don't make it an "std" module. I mean, you can document it in our "internal documentation". We don't need a "standard library" extension every time you add a debugging aid.

@timotheecour
Copy link
Member Author

timotheecour commented Jun 21, 2021

seems like all i need is move it stdx/vmutils and remove changelog entry then? (vmops may need some patching to support stdx, not sure yet)

regarding documentation, IMO everything should ideally be documented and docgen'd, including compiler (https://nim-lang.github.io/Nim/compiler/theindex.html), including runnableExamples wherever it makes sense; even for APIs that can change; doesn't mean end users need to see those docs, but those docs should be viewable somewhere.
Better docs for compiler/tools = lower barrier for contributions

stdx can have it's own doc/theindex, just like compiler now has (and listed in https://nim-lang.github.io/Nim/lib.html):

Manual
Standard library
extensions (stdx)
Index
Compiler docs
Fusion docs
devel, stable

@Araq
Copy link
Member

Araq commented Jun 24, 2021

Merging it as it is, will move it to lib/dist once we have that.

@Araq Araq merged commit 565e07a into nim-lang:devel Jun 24, 2021
@timotheecour timotheecour deleted the pr_vm_debug_simple branch June 24, 2021 16:48
@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Jul 7, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
…#18244)

* enable VM tracing in user code via  `{.define(nimVmTrace).}`

* add vmutils.vmTrace

* add vmTrace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants