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

Feature: upgrade macro evaluation strategy to use nimscripter #37

Merged
merged 20 commits into from
Jun 9, 2022

Conversation

ynfle
Copy link
Contributor

@ynfle ynfle commented May 23, 2022

This upgrades the representer to use https://github.com/beef331/nimscripter.

This module is a branch between Nimscript (a subset of nim that runs in
the VM) and the compiled nim. It embeds an interpreter in to the
executable, and doesn't require a nim compiler to be in path. The
primary advantage is the the bridge is built-in, so there is no need for
recompiling to create a representation for every exercise, as the
representer uses nim's macros which only function in the VM. Without
this, only the VM is used, but the executable has to be recompiled every
time.

This also greatly simplifies the testing process, as the representation
creation doesn't need to happen at compile to and injecting in. Rather,
it can happen at runtime with the VM bridge.

CI: update to use version 1.6.6 and refractor of the representer

https://github.com/jiro4989/setup-nim-action is used to install nim
using choosenim with the desired version.

Caching is utilized to not have to redownload the dependent packages
again and invalidates the cache when the nimble file changes. This is
the example used on the setup-nim-action repo README.md

Closes #9 as not relevant

ynfle added 2 commits May 24, 2022 01:19
This commit upgrades the representer to use
https://github.com/beef331/nimscripter.

This module is a branch between Nimscript (a subset of nim that runs in
the VM) and the compiled nim. It embeds an interpreter in to the
executable, and doesn't require a nim compiler to be in path. The
primary advantage is the the bridge is built-in, so there is no need for
recompiling to create a representation for  every exercise, as the
representer uses nim's macros which only function in the VM. Without
this, only the VM is used, but the executable has to be recompiled every
time.

This also greatly simplifies the testing process, as the representation
creation doesn't need to happen at compile to and injecting in. Rather,
it can happen at runtime with the VM bridge.
https://github.com/jiro4989/setup-nim-action is used to install nim
using choosenim with the desired version.

Caching is utilized to not have to redownload the dependent packages
again and invalidates the cache when the nimble file changes. This is
the example used on the `setup-nim-action` repo `README.md`
@ynfle ynfle requested review from ErikSchierboom and ee7 May 23, 2022 22:20
@ynfle ynfle requested a review from a team as a code owner May 23, 2022 22:20
@ynfle
Copy link
Contributor Author

ynfle commented May 23, 2022

Please review, although I need to look over everything again

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good. I'll let @ee7 do the actual Nim review. I restricted myself to commenting on the CI-related things.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
nim_representer.nimble Outdated Show resolved Hide resolved
@ErikSchierboom
Copy link
Member

@ynfle Is there any performance difference between this version and the previous one?

nim_representer.nimble Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
ynfle and others added 3 commits May 24, 2022 16:05
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I'm approving based on the GHA stuff.

@ynfle
Copy link
Contributor Author

ynfle commented May 24, 2022

Note docopt was added as a dependency here because before there was just compilation, so runtime args weren't possible. This can be split in to a separate PR or a separate commit.

@ynfle
Copy link
Contributor Author

ynfle commented May 29, 2022

@ynfle Is there any performance difference between this version and the previous one?

Yes. on my machine it is about a 2x performance boost. From ~1.6s on main and ~.75s with this latest commit on the PR.

As more normalizations are added, I think that there will be a less of a performance boost, as the primary difference isn't in the evaluation of the AST etc. but rather the startup time and "other things" that compiler does which we don't need and isn't embedded in our executable. The previous strategy was to recompile every time.

@ynfle
Copy link
Contributor Author

ynfle commented May 29, 2022

A fairer comparison would be a nimscript script which wouldn't need to recompile every time. A prototype with the following script ran at about ~.95s which is only a 1.25x speed up instead of 2x

import std/[json, macros, os, strformat, strutils]
import representer/[mapping, normalizations, types]

proc createRepresentation*(file: string): tuple[tree: NimNode, map: IdentMap] =
  var map: IdentMap
  let code = parseStmt(file)
  result = (tree: code.normalizeStmtList(map), map: map)

proc getTestableRepresentation*(contents: string, switch = false): SerializedRepresentation =
  let (tree, map) = createRepresentation(contents)
  result = (repr tree, $(if switch: %map.switchKeysValues else: %map))

proc getFileContents*(fileName: string): string = readFile fileName

func underSlug(s: string): string = s.replace('-', '_')

proc main() =
  let (tree, map) = getTestableRepresentation(getFileContents(paramStr(3) / underSlug(paramStr(2)) & ".nim"))
  if paramCount() == 5:
    let outDir = paramStr(4)
    writeFile outDir / "mapping.json", $map.parseJson
    writeFile outDir / "representation.txt", tree
  echo &"{tree = }\n{map.parseJson.pretty = }"

main()

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
bin/run.sh Show resolved Hide resolved
nim.cfg Show resolved Hide resolved
src/representer/loader.nims Outdated Show resolved Hide resolved
src/representer/normalizations.nim Outdated Show resolved Hide resolved
tests/tnormalizations.nim Show resolved Hide resolved
src/representer/types.nim Outdated Show resolved Hide resolved
src/representer/loader.nims Outdated Show resolved Hide resolved
Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see work on the representer - keep going! Nice idea to use nimscripter - it's an interesting application for it.

The main downside that I see is the relatively large increase in compilation time, but I think it's worth it.

This PR means that when we get a Dockerfile in this repo, we'll now add the representer binary, rather than the representer source file. And then we don't even need to have Nim installed in the image, right? So if we statically link the representer binary, we can have a tiny image that only contains enough to run run.sh, and the representer binary (looks like roughly 4.0 MiB after stripping, or 3.3 MiB with --passC:-flto).

src/representer/loader.nims Outdated Show resolved Hide resolved
src/representer/types.nim Outdated Show resolved Hide resolved
tests/tnormalizations.nim Outdated Show resolved Hide resolved
ynfle and others added 3 commits June 7, 2022 12:07
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
@ynfle ynfle force-pushed the feature-use-nimscripter branch from 03b414d to bc88fa5 Compare June 7, 2022 09:30
ynfle and others added 4 commits June 7, 2022 12:43
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
@ynfle ynfle force-pushed the feature-use-nimscripter branch 2 times, most recently from 9c9d8e5 to 27c9bad Compare June 7, 2022 09:58
ynfle and others added 4 commits June 7, 2022 17:56
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
@ynfle ynfle force-pushed the feature-use-nimscripter branch from 41d47f6 to 41a44c5 Compare June 9, 2022 00:23
@ynfle ynfle force-pushed the feature-use-nimscripter branch from 41a44c5 to f8042be Compare June 9, 2022 00:25
Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good - I find tests/tnormalizations.nim way more readable now - thanks.

I've created #38 and #39 to track the review comments that we could keep in mind for later.

Here's a few tiny nits - feel free to ignore.

Already approved before, but approving again for the recent changes.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
tests/tnormalizations.nim Outdated Show resolved Hide resolved
ynfle and others added 2 commits June 9, 2022 15:30
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
@ynfle ynfle merged commit 34ff2b0 into main Jun 9, 2022
@ynfle ynfle deleted the feature-use-nimscripter branch June 9, 2022 13:30
@ynfle
Copy link
Contributor Author

ynfle commented Jun 9, 2022

@ee7 @ErikSchierboom Which size level should this PR be labeled? x:size/massive?

@ErikSchierboom
Copy link
Member

We don't yet have official guidelines, but I'm fairly certain this would count a massive.

P.S. you should probably get into the habit of using the x:rep labels to assign reputation, as they're superseding the x:size labels for reputation awarding (they both work for backwards compatibility).

@ynfle ynfle added x:rep/massive Massive amount of reputation x:action/improve Improve existing functionality/content x:module/representer Work on Representers x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows) x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) labels Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/improve Improve existing functionality/content x:module/representer Work on Representers x:rep/massive Massive amount of reputation x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows) x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change declaration type of expected tree output from let to const
3 participants