Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upRun proc macro invocations in separate threads. #56058
Comments
eddyb
referenced this issue
Nov 19, 2018
Merged
Decouple proc_macro from the rest of the compiler. #49219
This comment has been minimized.
This comment has been minimized.
As a preliminary "fix" we could probably prohibit I'm also not quite sure what the reasoning behind this being undesirable is, and I think it would be good to include that in this issue so that the cost/benefit could be better analyzed. |
This comment has been minimized.
This comment has been minimized.
|
@Mark-Simulacrum Proc macros shouldn't have state between invocations, as we don't want to guarantee any particular execution order, and also incremental macro expansion wouldn't want to always re-run all the invocations. If necessary, we can make the incremental expansion integration opt-in, but I hope we don't have to (as most proc macros, especially derives, are stateless). |
This comment has been minimized.
This comment has been minimized.
|
Okay -- so mostly essentially non-technical reasons? That's what I expected. I think the main reason why I feel like thread isolation is not super helpful is that it seems like then people will just move to |
This comment has been minimized.
This comment has been minimized.
|
@Mark-Simulacrum I think we need to be more clear about not guaranteeing execution order in documentation and such; is this even documented anywhere right now? |
This comment has been minimized.
This comment has been minimized.
|
@Mark-Simulacrum One thing I forgot to mention is that the But with #49219, methods will panic (including |
added a commit
that referenced
this issue
Nov 24, 2018
added a commit
that referenced
this issue
Nov 24, 2018
added a commit
that referenced
this issue
Nov 24, 2018
added a commit
that referenced
this issue
Nov 25, 2018
added a commit
that referenced
this issue
Nov 26, 2018
added a commit
that referenced
this issue
Nov 26, 2018
added a commit
that referenced
this issue
Nov 26, 2018
added a commit
that referenced
this issue
Nov 26, 2018
added a commit
that referenced
this issue
Nov 26, 2018
added a commit
that referenced
this issue
Nov 26, 2018
added a commit
that referenced
this issue
Nov 27, 2018
This comment has been minimized.
This comment has been minimized.
jaynagpaul
commented
Nov 29, 2018
|
Is there any possible workaround for maintaining state between proc macros invocations? If not are there plans for a solution? |
This comment has been minimized.
This comment has been minimized.
|
What is the use case for that? |
This comment has been minimized.
This comment has been minimized.
jaynagpaul
commented
Nov 29, 2018
•
|
@Mark-Simulacrum A mini web framework, looked at a couple of solutions, the two I've found so far is generating functions which is used by rocket and reset-router. Currently sparkles is using thread local state and it is what I was leaning towards before finding this issue. It has the added benefit of avoiding reset-router + rocket's namespacing problem |
This comment has been minimized.
This comment has been minimized.
|
Is what you want to do possible if we randomized the order we expand things in? If not, you shouldn't do it. OTOH, there may be something you could do by having a proc macro attribute on a module or entire crate (I don't think we currently support it, but it would allow more interesting transformations). |
eddyb commentedNov 19, 2018
•
edited
#49219 introduces
proc_macro::bridge::server::{ExecutionStrategy,SameThread,CrossThread}.SameThreadhad to be used becauseCrossThreadwas a significant performance regression.Ideally, we'd use
CrossThread, which spawns a thread for each invocation, to prevent (and discourage) proc macros from using TLS for state between invocations.But we'd have to figure out how to make it not regress performance too much, if at all possible.
(it could be the use of channels, which are overkill since they never have more than one value)
cc @dtolnay @alexcrichton @petrochenkov
(TODO: update with regressions, if any, after the cross-thread crater run finishes on #49219)