-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: use the tokio task instead of the native thread to poll each isolate #244
Conversation
As I said in the conversation of the previous PR, I've modified integration tests to be compatible with the new policies. Especially, I've made changes for the tests to be run on the |
I do not have permission to restart the github test action, but on my local machine, the flakes (that's the same flake with the github runner), which occurred if I ran tests with Valgrind are gone. |
NOTE: message
|
🙄😅 |
It's weird 🧐 |
Okay, I think I got it. This might be a PKU problem like I submitted the PKU-related PR before. |
This PKU-related SIGSEGV only happens when JIT compilation is enabled, so the tests that aren't hot enough when V8 to see may have been lucky enough to pass. |
Invocation timing must be adjusted so that V8 platform initialization function does not depend on the CLI. |
🙄 |
Okay, I found another point that might have triggered the SIGSEGV. (and able to reproduce on my x86_64 machine, but not aarch64 machine) |
a4270f9
to
1621c71
Compare
🎄 |
c239377
to
0788117
Compare
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.
So far so good. I'm looking at the integration tests it seems like it's only testing successful scenearios? can you correct me if im wrong? If that's the case though, it would be good to test the failure scenearios like when the timeout is called for example..
@andreespirela That's a good point! I'll write the tests for various failure scenarios. |
I have not written any integration tests, as I mentioned in the previous PR, I've just added the flow control routines to existing integration tests 😋 |
Yep that's all good, maybe it doesn't need to be a whole new test completely, maybe you can part from existing ones, just as long as we're testing the successful and failure scenearios, it's all good. |
@andreespirela |
BTW, I have an idea for the failure scenarios because I experienced various failures while stress testing 😋 |
Note: After this PR all tests that make the worker or worker pool inside the body must run serial. This PR makes the tokio runtime global for spinning up the tasks. The Isolates are very sensitive using thread local storage, so they should run serially. |
In the morning in my local time (South Korea, KST), I'll be writing tests for timeout, intentional connection reset by the peer, and terminating the request due to CPU time being exhausted (also including wall clock timeout). |
I just finished writing the validation tests for various request failure scenarios. |
Characteristics
|
🧐 Request throughput is nearly 2x improved 😋 |
@nyannyacha Insane PR. I'm very happy with this. I will approve it but let's wait for @laktek to also approve it. I've tested it locally and everything makes sense, but @laktek is our CPU timer rockstar . |
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.
LGTM
We should call `Isolate::Enter` because we must bind the isolate to the thread when evaluating the mod initially, but that doesn't mean it should only be bound when evaluating. Naturally, we should invoke `Isolate::Enter` even when the v8 isolate enters its loop again. What a silly mistake again 😫
…various error messages
d3df2a5
to
0f6bdfd
Compare
rebased |
At the time of invoking `DenoRuntime::new()`, it is a time that already detached from the main thread. The function initializes the V8 platform should be invoked only from the main thread, so it's not right. (cherry picked from commit 989867a)
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.
Thanks again for this contribution and taking time to leave elaborate comments the changes. Sorry for the delay in getting it merged 😓
Renamed the prefix to |
🎉 This PR is included in version 1.32.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What kind of change does this PR introduce?
Refactor
Description
This is a continuation of #241 .
The PR refactors the part that spawns threads for the isolate into Tokio-based lightweight threads1.
In my observation, the PR results in significant improvements in terms of memory consumption and request throughput compared to v1.30.0.
However, the memory leak issue will continue to exist as long as #240 is not merged, so expect OOM to be unavoidable over time.
Minor changes
cpu_timer
crate. This resulted in running tests being more stabilized.WorkerRequestCancelled
error instead of anInvalidWorkerResponse
error. It is only raised when the script execution reaches the CPU/Wall-clock/Memory limit.2Footnotes
The thread pool size for the isolates can be adjusted through
EDGE_RUNTIME_WORKER_POOL_SIZE
environment variable. The default thread pool size follows the return value ofstd::thread::available_parallelism
. ↩If it is too small to maintain the isolation in stable, It can raise various errors including the
WorkerRequestCancelled
. ↩