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

Reuse Hy compiler instances #1700

Merged
merged 3 commits into from
Nov 29, 2018

Conversation

brandonwillard
Copy link
Member

Every call to hy_eval and hy_compile will create a new HyASTCompiler instance, which incurs Hy standard library setup costs each time. When calls to these functions occur more than once (e.g. by use of HyREPL and eval-and-compile), the aforementioned work is performed unnecessarily for the same module. These changes avoid that unneeded and error-prone repetition.

Copy link
Member

@Kodiologist Kodiologist left a comment

Choose a reason for hiding this comment

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

hy_eval is user-visible (as eval), so update its documentation in core.rst and NEWS.

@brandonwillard
Copy link
Member Author

In order for a compiler instance to simply [re]use itself when calling hy_eval (e.g. during eval-and-compile), we need to make the compiler "stateless". Members like imports and temp_if attach state to a compiler instance that don't allow its immediate reuse.

To replace imports, my plan of attack involves more consistent tracking/use of Result objects and the imports in those. Basically, we can—and should—keep all "state" within Result objects. This looks like it will involve an interface-like change to the compiler, though; namely, that all HyASTCompiler.compile_* methods must return and take Result objects. While that's often the case (well, the former at least), it's not always true, and this inconsistency isn't good for the compiler's design.

For instance, HyASTCompiler.compile_symbol doesn't return a Result object, but it can be made to do so, and—with that change—it can assign stdlib auto-imports to the current state of compilation (via Result.imports) instead of the actual compiler instance. Likewise, with that in place, we can make HyASTCompiler.imports_as_stmts produce AST for a Results's imports and return those in another Result object—bringing it more in-line with its adjacent compile_* methods. Additionally, the logic currently in hy_compile could then be moved to HyASTCompiler.imports_as_stmts and disentangle essential compilation logic from hy_compile.

At the very least, these changes would get us closer to a compiler that better isolates its functionality and is significantly easier to extend and debug.

Regarding HyASTCompiler.temp_if, I'm having a hard time understanding exactly what its functional "intentions" are and the exact scope/constraints of its role. I can see that it's attempting to avoid the unnecessary use/creation of temporary variables in an if*, but how it's using the compiler state to do this isn't very clear or well contained. I can't tell whether or not the addition of a temp_if field to Result would work, or if there's something much simpler that would avoid such an undesirable addition.

Somewhat related issues: #1542, #1482. Also, this comment in #1697.

@brandonwillard
Copy link
Member Author

@Kodiologist, good call; will do.

@Kodiologist
Copy link
Member

I'm afraid this isn't rebasing cleanly after merging #1699. Could you take a look?

@brandonwillard
Copy link
Member Author

Yeah, one minute.

These changes make `HyREPL` use a single `HyASTCompiler` instance, instead of
creating one every time a valid source string is processed.

This change avoids the unnecessary re-initiation of the standard library
`import` and `require` steps that currently occur within the module tracked by a
`HyREPL` instance.

Also, one can now pass an existing compiler instance to `hy_repl` and
`hy_compiler`.

Closes hylang#1698.
@brandonwillard brandonwillard force-pushed the reuse-compiler-instances branch from c117f86 to 3d0659b Compare November 28, 2018 22:35
@brandonwillard
Copy link
Member Author

OK, that should do it.

@Kodiologist
Copy link
Member

Kodiologist commented Nov 28, 2018

Nice. One more thing: the documentation of eval in core.rst. I know, it's a problem that we have both docstrings and the redundant documentation in those files.

@brandonwillard
Copy link
Member Author

Didn't notice that; I'll update it.

Otherwise, it seems like we should really get started on #1044.

@Kodiologist
Copy link
Member

Replace "fifth" with "fourth" and I'll merge.

Yep.

@brandonwillard brandonwillard force-pushed the reuse-compiler-instances branch from 00009b6 to 690416b Compare November 28, 2018 23:08
@Kodiologist Kodiologist merged commit 9efd2a9 into hylang:master Nov 29, 2018
@brandonwillard brandonwillard deleted the reuse-compiler-instances branch November 29, 2018 22:41
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.

2 participants