feat: Upgrade to V8 13.4 #1070
Closed
feat: Upgrade to V8 13.4 #1070
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Another backtrace: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1070 +/- ##
==========================================
+ Coverage 81.43% 81.47% +0.03%
==========================================
Files 97 101 +4
Lines 23877 27966 +4089
==========================================
+ Hits 19445 22784 +3339
- Misses 4432 5182 +750 ☔ View full report in Codecov by Sentry. |
ry
reviewed
Feb 7, 2025
| @@ -0,0 +1,2 @@ | |||
| [env] | |||
| RUST_TEST_THREADS = "1" | |||
There was a problem hiding this comment.
we get random crashes (related to shared ro heap) without it. like this one:
test integration::dyn_import_circular ... FAILED
#
# Fatal error in , line 0
# Check failed: IsFreeSpaceOrFiller(filler).
#
#
#
#FailureMessage Object: 0x7f3d28bfbba0
==== C stack trace ===============================
Can only use one JsRuntime at a time.
There was a problem hiding this comment.
hmmm should this be a blocker to landing it? seems bad.
There was a problem hiding this comment.
yeah, and i think this is partly the same reason we can't have TSC snapshot anymore right?
ry
reviewed
Feb 7, 2025
Comment on lines
+29
to
+48
| /* | ||
| test(function testWithNul() { | ||
| // deno-fmt-ignore | ||
| const fixture1 = [ | ||
| 0xf0, 0x9d, 0x93, 0xbd, | ||
| 0xf0, 0x9d, 0x93, 0xae, | ||
| 0xf0, 0x9d, 0x94, 0x81, | ||
| 0xf0, 0x9d, 0x93, 0xbd, | ||
| 0 | ||
| ]; | ||
| const res = Array.from(Deno.core.encode("𝓽𝓮𝔁𝓽\0")); | ||
| console.log(res); | ||
| assertArrayEquals( | ||
| res, | ||
| fixture1, | ||
| ); | ||
| assertEquals(Deno.core.decode(new Uint8Array(fixture1)), "𝓽𝓮𝔁𝓽\0"); | ||
| }); | ||
| */ | ||
|
|
There was a problem hiding this comment.
This test should be uncommented, not removed
bartlomieju
commented
Feb 7, 2025
serde_v8/de.rs
Outdated
There was a problem hiding this comment.
We should revert changes to the string handling for now
There was a problem hiding this comment.
I think there is a missing export in rusty_v8?
error[E0432]: unresolved import `v8::WriteOptions`
--> core/runtime/ops.rs:16:5
|
16 | use v8::WriteOptions;
| ^^^^^^^^^^^^^^^^ no `WriteOptions` in the root
|
apart from using diff --git a/testing/checkin/runner/mod.rs b/testing/checkin/runner/mod.rs
index f136a8b..a74a021 100644
--- a/testing/checkin/runner/mod.rs
+++ b/testing/checkin/runner/mod.rs
@@ -24,6 +24,7 @@ use std::sync::mpsc::channel;
use std::sync::mpsc::RecvTimeoutError;
use std::sync::Arc;
use std::sync::Mutex;
+use std::sync::OnceLock;
use std::time::Duration;
mod extensions;
@@ -92,13 +93,27 @@ impl TestData {
}
}
+static SNAPSHOT: OnceLock<&'static [u8]> = OnceLock::new();
+
pub fn create_runtime(
parent: Option<WorkerCloseWatcher>,
additional_extensions: Vec<Extension>,
) -> (JsRuntime, WorkerHostSide) {
let (worker, worker_host_side) = worker_create(parent);
- let snapshot = snapshot::create_snapshot();
- let snapshot = Box::leak(snapshot);
+
+ let snapshot = SNAPSHOT.get_or_init(|| {
+ match fork::fork() {
+ Ok(fork::Fork::Child) => {
+ let snapshot = snapshot::create_snapshot();
+ std::fs::write("CLI_SNAPSHOT.bin", &snapshot).unwrap();
+ }
+ Ok(fork::Fork::Parent(pid)) => fork::waitpid(pid).unwrap(),
+ e => panic!("Failed to fork"),
+ };
+ let snapshot = std::fs::read("CLI_SNAPSHOT.bin").unwrap();
+ Box::leak(snapshot.into_boxed_slice())
+ });
+
let mut runtime =
create_runtime_from_snapshot(snapshot, false, additional_extensions);
runtime.op_state().borrow_mut().put(worker); |
devsnek
reviewed
Feb 10, 2025
| @@ -0,0 +1,2 @@ | |||
| [env] | |||
| RUST_TEST_THREADS = "1" | |||
There was a problem hiding this comment.
you can use cargo-nextest to run each test in a separate process.
devsnek
reviewed
Feb 10, 2025
| @@ -97,6 +97,24 @@ pub fn create_runtime( | |||
| additional_extensions: Vec<Extension>, | |||
| ) -> (JsRuntime, WorkerHostSide) { | |||
| let (worker, worker_host_side) = worker_create(parent); | |||
|
|
|||
| /* TODO: remove, this is a hack | |||
There was a problem hiding this comment.
same nextest deal here. unless we get groups landed, but this would still fail in configs without groups.
|
Closing in favor of #1076 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.