-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cache context base base in quote driver #4643
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
Conversation
bf31454
to
e959e89
Compare
e959e89
to
2a8f16b
Compare
2a8f16b
to
486007c
Compare
import dotty.tools.dotc.tastyreflect.TastyImpl | ||
|
||
class QuoteDriver extends Driver { | ||
import tpd._ | ||
|
||
def run[T](expr: Expr[T], settings: Settings[Run]): T = { | ||
private[this] val contextBase: ContextBase = new ContextBase |
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.
I don't think this is thread-safe
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.
It is not and not intended to be. Each thread should generate its own toolbox. I will add documentation and thick how to check that they do not use the same compiler in two threads.
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.
Decided to make it thread-safe by synchronization instead. If someone wants parallelism they should create several instances of the Toolbox
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.
Decided to make it thread-safe by synchronization instead.
I don't see any synchronization logic in the code.
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.
I must have lost that commit in one of the reading. I will add it again.
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.
Searching for synchronized
still finds nothing here?
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.
It is in #4746
b56fda2
to
80266ae
Compare
Rebased |
Should be re-enbled in scala#4643
Should be re-enbled in scala#4643
* Cache the driver itself * Cache context base in the driver * Addapt toolbox settings * Use cached toolbox in tests
This reverts commit ce3f7b4.
80266ae
to
f781701
Compare
Rebased and reenabled test |
Also had a look. Seems good. |
This enables caching the compiler used of call
Expr.run
.