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

Comptime module system #56

Closed
wants to merge 11 commits into from
Closed

Comptime module system #56

wants to merge 11 commits into from

Conversation

sioonhho
Copy link

@sioonhho sioonhho commented Jul 27, 2021

Hey hey, saw you were attempting a rewrite on the module system and thought I'd give my take on it.

Changes

The internal compile fennel script has been removed and replaced by calling the compiler directly from the CLI, and the macros file has also been removed since it's been replaced by the compiler plugin.

Some important things that have changed in my Fennel fork:

  • There is now a module field in the compiler scope, to ensure that there is a persistent state to place module related data. (this is be elaborated on further down)
  • The :call hook now passes in parent, opts, and compile1 as well as ast and scope.
  • The opts variable above has a new field, filename, set to utils.root.options.filename to ensure that we can set *file* without having to prepend it to the string being compiled.
  • The (lua ...) special function has been fixed to work as intended. This is needed for the new implementations of defonce[-].

(autoload alias name)

Autoload is the only form with a different public facing definition, however this shouldn't be too much of a problem as most autoload references are in module definitions and those will resolve themselves.

This has been slightly rewritten to generate cleaner and faster Lua. The alias is initially assigned a blank table that when called, indexed, or tset calls set-forcibly! on the alias with the result of (require name), instead of just updating the state in the table. This results in only calling ensure once, and each subsequent reference to alias accesses the required module directly.

(module name ...)

This form has also been changed quite a bit.

Here scope.module is set to its initial state to be used by the new implementations described below.

After stringifying the new-local-fns, that table is partitioned by checking if the key is :autoload. If there are any autoloads then aniseed.autoload is imported and the necessary locals are compiled, the other new-local-fns are handled in the same way as before.

Throws a compiler error if no name is provided.

(def[-] name value)

The new plugin implementation takes an optional custom value? argument, otherwise the third ast element is used. It also takes a private? boolean argument to signify whether or not the def or def- form was invoked, private? here determines the proper sub table we need to update in scope.module (locals def- or public def).

This is the first instance of compile1 so far, and it allows us to inject code before the ast currently being processed. Taking advantage of it leads to much cleaner and less Lua at the cost of slightly longer compile time. (local name value) is compiled regardless of private?'s value, as is setting the ast to (tset *module* :aniseed/locals (tostring name) name). With this approach, we can leverage compile1 to achieve the same result as a let statement, but with less Lua.

If private? is false, def was invoked, then (tset *module* (tostring name) name) is compiled after the initial local call.

Throws a compiler error if no name or value are provided.

(defn[-] name args body)

Pretty straightforward, nothing too fancy in this one, also takes in a private? argument to be propagated down to def-form. defn-form just builds an anonymous function with the supplied args and body, and hands it off to def-form as an optional custom value.

Throws a compiler error if no args or body are provided. No name falls through to the assert-compile in def-form.

(defonce[-] name value)

This form has what I think is one of the more important changes in this rewrite. Previously when defonce[-] was called, and the value was already defined, it would still generate the same code as if it hadn't been defined yet resulting in a lot of redundancy. In reality this should've been a no-op, and the fixed (lua ...) special function makes that possible.

private? is used to determine the proper sub table of scope.module to check if the variable has already been defined. If it has, then the ast is replaced with (lua nil) which emits zero Lua, otherwise the ast and private? are handed off to def-form.

Throws a compiler error if no name is provided. No value falls through to the 'assert-compileindef-form`.

(deftest name body)

Deftest is mostly the same, but with some slight optimizations. The test function ast is built and passed to compile1. If scope.module.has-tests is true then then (tset *module* :aniseed/tests (tostring name) test-fn-sym) is compiled, otherwise that scope variable is set to true and instead (tset *module* :aniseed/tests {(tostring name) test-fn-sym}) is compiled.

Throws a compiler error if no name or body are provided.

(time body)

Time has been changed quite a bit as well, instead of inlining the time function for each call, the first time time is called the timing function is compiled, and the symbol used is set in the scope for later calls. Then the body ast, which just wraps what was called with time in a do block, is built, and the ast is set to (time-fn-sym body-ast).

Throws a compiler error if no body is provided.


While there is a slight cost to compile time as seen in the table below generated using hyperfine:

Command Mean [s] Min [s] Max [s] Relative
make compile master 1.266 ± 0.032 1.238 1.333 1.00
make compile comptime-module-system 1.372 ± 0.028 1.331 1.414 1.08 ± 0.04

The resulting code is also much smaller and faster. With my neovim configuration, the master branch generated 509 lines of lua, this fork generates 225.

Here are the startup times with my configuration compiled with the two branches:

compiled with master branch

Command Mean [ms] Min [ms] Max [ms] Relative
nvim --headless -c q 311.6 ± 46.2 210.3 354.2 1.00

compiled with comptime fork

Command Mean [ms] Min [ms] Max [ms] Relative
nvim --headless -c q 120.5 ± 2.7 118.4 127.8 1.00

I've also been compiling my Fennel config with this fork since the first commit that made it usable, and I haven't had any issues with it, so it doesn't just perform well in practice and tests.

Olical and others added 8 commits July 23, 2021 11:44
and the internal compile.fnl script following the suggestions
in the comments. Uses my fork of Fennel that patches the `lua`
special, and adds a few small changes to support this rewrite.
Rewrote autoload to generate slightly less code. Slightly
modified the tests to properly reflect the changes.
on module-system-tests to include compilation tests and then some.
whether the collection type was a sequence or table. Cleaned up the
process by which `compile1` is called. Some slight changes to tests, and
general code cleanup.
@sioonhho
Copy link
Author

sioonhho commented Aug 1, 2021

As of the latest commit, (module ...) now accepts all forms of importing other files that I'm aware of, (include, require, import-macros, require-macros, and autoload). These are handled explicitly by key as opposed to the type of the value. Also, as an emergent behavior from the design of this module system, you're now able to destructure the table returned from a require call in (module ...). This generates the same Lua as if you were to destructure it after the module declaration, and lets you write less and cleaner Fennel. I spent quite a lot of time trying to get this destructuring-in-module support working for autoload, but it's really tricky, so for now autoload only supports symbols like it did before. I'm definitely going to continue looking into it though, since I think it'd be a neat feature.

Other than that last bit, this fork does what I set out to write, so whenever you're ready to review fire away.

@Olical
Copy link
Owner

Olical commented Aug 2, 2021

Hmm I'd be careful with this! I'm not sure if I'll continue with this branch / approach as it stands! I'm currently leaning towards improving the evals to make them more contextual to reduce the need for clever macros. So people don't NEED to use my special syntax flavour, instead it's an optional thing for people who like it.

The macros could then get simpler (I think). I don't know if relying on the compiler plugin infrastructure is a good idea right now either, I think the docs make it pretty clear that it's very very alpha and prone to breakage / changes. So sticking with macros but making the non-macro approach viable then simplifying the macros is probably the way to go.

I'll still be sure to look through your ideas, but they might not fit in this incarnation. If there's changes you'd like to make or improvements you'd like to see we can discuss them in an issue to see if we're both happy then target the existing macros on the master / develop branch.

@sioonhho
Copy link
Author

sioonhho commented Aug 5, 2021

Fair points for sure, you mention improving evals and making the non-macros approach viable. What exactly do you mean by those? Seeing as users don't have to use the module system to take advantage of aniseed. I'm just curious as to the route you're wanting to take with this is all 😁.

@zetashift
Copy link

zetashift commented Aug 7, 2021

How does one get started with using your fork?
Currently running into this error:

Never mind, gonna play around with it, thank you :P

@Olical
Copy link
Owner

Olical commented Aug 8, 2021

I basically want to replace my current eval calls with a stateful REPL that I instantiate per-buffer. This will allow me to keep state local to each buffer, ensure evals persist state between them and drastically simplify the macros since all of their overly complex code exists to get around the problem of each eval being performed in a clean environment.

So I want to make this behaviour possible:

  • Eval (local foo 10)
  • Then eval separately (+ foo 1) to get 11

Right now that'll return an error since every eval happens in a clean environment, so the local is just thrown away as soon as the eval is complete. That limitation is the reason the macros are extremely complicated, so as soon as I can change this behaviour I can massively simplify them into a light sugar on top of Fennel instead of a whole cross-eval-state-transfer mechanism.

@Olical
Copy link
Owner

Olical commented Aug 23, 2021

This is finally clicking for me, sorry I haven't really been present. I think I'm accidentally working towards the same goal / changes but in a roundabout way.

I've been working on how Conjure + Aniseed eval Fennel within the Neovim LuaJIT. I've implemented a REPL based flow on the develop branch of them both, so Conjure on develop can now eval Fennel properly without any Aniseed macros.

So stuff like

(local foo 10)
(+ 10 foo)

Evaluating those two lines with separate <prefix>ee will actually work and remember the local you defined. There's a REPL per file, so as you swap buffers you get an appropriate REPL context, as you'd expect.

All of this to say that I'm working towards MUCH lighter macros (maybe a fennel plugin, although I found working with that awkward / had issues / it's unstable still) that don't need to be anywhere near as smart due to the better eval mechanics.

As part of that I want to move more code to comp time anyway. So I'm working towards this goal and will incorporate your work if I can, although I don't want to require a fork of Fennel (I'm actually using a fork right now for a minor bug fix that I have submitted upstream).

So, I hope this heads up was informative and helpful 😅 still no concrete actions coming out of it, but an update all the same!

@Olical
Copy link
Owner

Olical commented Aug 23, 2021

Do you intend / are you trying to get your Fennel fork improvements merged upstream out of interest?

@Olical
Copy link
Owner

Olical commented Sep 21, 2021

I think I have to close this now since the current dev branch has moved on and have (I think?) achieved the same goals? I'm now generating extremely minimal code with the module macro system and it's not even a requirement for using Aniseed anymore. Thank you for putting in all of this time and research though! I'm open to feedback, speed testing and critiques of the new system as well as improvements if they build on what's there now :)

@Olical Olical closed this Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants