-
Notifications
You must be signed in to change notification settings - Fork 236
Use JavaPersistantWorker, create new kotlinc every invocation #703
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
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.
Nice clean up. Just some non-blocking nits and questions.
|
||
load( | ||
"@build_bazel_rules_android//android:rules.bzl", | ||
"android_ndk_repository", |
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.
yass qween
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 ndk rules are a mess. Don't work past 27 afaict.
fun exec(errStream: java.io.PrintStream, vararg args: kotlin.String): ExitCode { | ||
class BazelK2JVMCompiler { | ||
fun exec(errStream: java.io.PrintStream, vararg args: String): ExitCode { | ||
System.setProperty("zip.handler.uses.crc.instead.of.timestamp", "true") |
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.
This is painful (that it's set as a global property in an exec() method I mean, nothing to be done about that if we want to ensure it, I suspect)
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 also doesn't work. Will remove.
while (true) { | ||
WorkResponse.parseDelimitedFrom(stdOut.input) | ||
?.let(responses::add) | ||
?: break |
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.
Ok, I like this sneaky little pattern.
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's very jetbrains kotlin. Been reading too much code from there.
println("sequence $this") | ||
} | ||
} | ||
}.associateBy { wr -> |
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.
wr? I know you didn't name it here, but can you drive-by rename with something more clear?
fun waitForStdOut() { | ||
while (stdOut.input.available() != 0) { | ||
Thread.onSpinWait() | ||
Thread.sleep(1) // don't starve other processes. |
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.
Doesn't doing a Thread.sleep(1)
more or less do as much as onSpinWait()
or more? As in, if you have sleep(1)
do you still need onSpinWait()
?
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.
Hm. I did this as the tests kept hanging.
* @param captureIO to avoid writing stdout and stderr while executing. | ||
* @param cpuTimeBasedGcScheduler to trigger gc cleanup. | ||
*/ | ||
class JavaPersistentWorker( |
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.
Why the rename here? Is this naming relevant to "java vs. js or llvm/native"? Or java in some other sense?
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.
Ah, I had named the other Kotlin for coroutines. Will fix.
4b5775e
to
d4d3d41
Compare
Switch to java implementation of the PersistentWorker, move the coroutine impl to side pending investigation
* Set IO to track thread local when capturing stdout and stderr * Small threading fixes in the test structure (threads push things harder tha coroutines)
d4d3d41
to
92890c1
Compare
K2JVMCompiler
on each invocation to avoid keeping thread localszip.handler.uses.crc.instead.of.timestamp
as a hail mary to prevent corruptionandroid_ndk
from examples, as no example uses it... and it's broken for later versions of the android ndk.