-
Notifications
You must be signed in to change notification settings - Fork 102
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
feed quasiquote variables in as arguments #30
Conversation
interpolating these into the AST gives the optimizer too much flexibility for a benchmark, that can mean the compiler will be able to just constant-fold away the work
Thanks! This is a really valuable change, and will only get more useful as the compiler gets smarter. |
@@ -203,8 +220,14 @@ macro benchmarkable(args...) | |||
end | |||
deleteat!(params, delinds) | |||
|
|||
quote_vars = Expr[] | |||
core = quasiquote!(core, quote_vars) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test failure comes from catching the possible (though extreme) case where core
here is a Symbol
.
Wouldn't it be better to automatically do this for any bindings from the outer scope? I.e. instead of having to quasiquote globals to measure the right thing, just do it automatically. |
This was originally the case when this package was still in its initial development phase, but after spending some time with it, I ended up going with the current behavior. It's less magical, less bug-prone, and encourages users to be explicit with regards to (and thus think carefully about) variable scoping in the benchmark expression. I did think about "inverting" the current behavior (e.g. variables would be local by default and global if you "interpolated" them), but that ended up being confusing to use/implement via interpolation syntax. |
not sure what it means to benchmark a constant value, but need to handle this case anyways
Thanks jrevels. I leave it up to you what you want to do with this. With the change you mentioned, it now passes CI (it looks like the mac buildbots had network issues). I just mostly wanted to see if it was possible / how it would work. This is probably not a complete unquote implementation (it doesn't handle nesting, although perhaps it doesn't need to), but I thought it might be interesting to play with. |
interpolating these into the AST gives the optimizer too much flexibility
for a benchmark, that can mean the compiler will be able to just constant-fold away the work
this rewrites the quasiquote interpolation variables as setup variables, rather than capturing their values directly in the function, thereby ensuring the compiler can't optimize the function based upon their values