Upgrade V8 to 8.9.255.3 #567

bnoordhuis · 2020-12-28T14:24:42Z

No description provided.

src/binding.cc

bnoordhuis

@piscisaureus This needs some careful review.

src/scope.rs

bnoordhuis · 2020-12-28T18:24:08Z

tests/test_api.rs

-  assert_eq!(false, isolate.terminate_execution());
-  assert_eq!(false, isolate.cancel_terminate_execution());
-  assert_eq!(false, isolate.is_execution_terminating());
+  drop(isolate);
+  assert_eq!(false, handle.terminate_execution());
+  assert_eq!(false, handle.cancel_terminate_execution());
+  assert_eq!(false, handle.is_execution_terminating());


I think this is a bug fix because the test shouldn't have been passing before the upgrade, AFAICT: without the drop(isolate), the isolate is alive and well, there's no reason isolate.terminate_execution() should return false.

The commit that added this test had a drop in it but that got removed somewhere along the way.

tests/test_api.rs

piscisaureus · 2020-12-28T18:56:29Z

Noted - will review carefully.

bartlomieju

ResolveCallback and InstantiateModule are getting deprecated with current arguments:

 using ResolveCallback V8_DEPRECATE_SOON("Use ResolveModuleCallback") =
      MaybeLocal<Module> (*)(Local<Context> context, Local<String> specifier,
                             Local<Module> referrer);
  using ResolveModuleCallback = MaybeLocal<Module> (*)(
      Local<Context> context, Local<String> specifier,
      Local<FixedArray> import_assertions, Local<Module> referrer);

  /**
   * Instantiates the module and its dependencies.
   *
   * Returns an empty Maybe<bool> if an exception occurred during
   * instantiation. (In the case where the callback throws an exception, that
   * exception is propagated.)
   */
  V8_DEPRECATE_SOON(
      "Use the version of InstantiateModule that takes a ResolveModuleCallback "
      "parameter")
  V8_WARN_UNUSED_RESULT Maybe<bool> InstantiateModule(Local<Context> context,
                                                      ResolveCallback callback);
  V8_WARN_UNUSED_RESULT Maybe<bool> InstantiateModule(
      Local<Context> context, ResolveModuleCallback callback);

@bnoordhuis could you port ResolveModuleCallback in this PR?

bnoordhuis · 2020-12-29T10:13:46Z

I'll leave that for another PR, this PR is already biggish and there's no rush. They're marked V8_DEPRECATE_SOON(), meaning they'll be around for at least the lifetime of 8.9 and 9.0.

piscisaureus · 2020-12-30T04:04:07Z

src/isolate_create_params.rs

@@ -199,7 +207,7 @@ pub(crate) mod raw {
  impl Default for CreateParams {
    fn default() -> Self {
      let size = unsafe { v8__Isolate__CreateParams__SIZEOF() };
-      assert_eq!(size_of::<Self>(), size);
+      assert!(size <= size_of::<Self>());


It's a bit unfortunate that we will no longer find out about CreateParams changes at compile time.
We always use LLVM's libc++, so is it really not possible to keep the struct sizes on both sizes exactly in sync?

If it turns out that maintaining a Rust definition of CreateParams is too burdensome because it changes a lot, we should move away from it and add bindings for C++ new/delete CreateParams() and add getters/setters for the various properties.

We always use LLVM's libc++

env GN_ARGS="use_custom_libcxx=true" V8_FROM_SOURCE=1 cargo build works, AFAICT, and I expect someone somewhere sooner or later will want to use that.

Another problem is that the size of vectors is different in libstdc++ debug builds.

env GN_ARGS="use_custom_libcxx=true" V8_FROM_SOURCE=1 cargo build works

Not really a surprise since use_custom_libcxx=true is the default. The "custom libcxx" refers to LLVM libc++.

Using use_custom_libcxx=false most certainly does not work on Windows, I can't speak to what the situation is on Linux but I suspect you'd have to do some work there too.

Haha, I even edited my comment to change false to true because I wasn't sure which one did what.

Both values produce a working build (seemingly, anyway, on linux), so yeah, I don't think it's safe to assume too much about STL type sizes.

piscisaureus · 2020-12-30T04:05:48Z

src/isolate_create_params.rs

@@ -187,6 +191,10 @@ pub(crate) mod raw {
    pub only_terminate_in_safe_scope: bool,
    pub embedder_wrapper_type_index: int,
    pub embedder_wrapper_object_index: int,
+    pub cpp_heap_params: SharedPtr<CppHeapCreateParams>,
+    // This is an std::vector<std::string>. It's usually no bigger
+    // than three or four words but let's take a generous upper bound.


Just wanted to point out that this is only safe because the CreateParams struct is always "stored" in rust and "loaded" in C++. If it were the other way around you'd want to make it a "generous lower bound".

bnoordhuis · 2020-12-30T10:30:46Z

@piscisaureus Your comment got buried, probably because you pushed a commit, but for posterity:

I don't think we want to enter/exit isolates in the scope system as it is indeed very performance sensitive.

Also, the V8 API until now only really requires the use of TLS in one dusty corner^*, the Exception::(Type|Range|Syntax)?Error() constructors for JavaScript exception objects. See exception.rs how it is dealt with.

I also noticed that the tests pass just fine in release mode, I'm only seeing some failures in debug mode.
So I'd suggest to figure out where Isolate::Current() is being used - probably inadvertently - and fix those call sites.

(*) A number of older APIs fall back to a TLS lookup if you pass an empty Local<Context> handle, but the rust_v8 API doesn't allow this and I see no need to move in that direction.

The one I'm aware of is the ScriptOrigin constructor. However, there are several places where V8 creates one internally.

piscisaureus · 2020-12-31T00:10:20Z

@piscisaureus Your comment got buried, probably because you pushed a commit, but for posterity:

I don't think we want to enter/exit isolates in the scope system as it is indeed very performance sensitive.
Also, the V8 API until now only really requires the use of TLS in one dusty corner, the Exception::(Type|Range|Syntax)?Error() constructors for JavaScript exception objects.
I also noticed that the tests pass just fine in release mode, I'm only seeing some failures in debug mode.
So I'd suggest to figure out where Isolate::Current() is being used - probably inadvertently - and fix those call sites.

The one I'm aware of is the ScriptOrigin constructor. However, there are several places where V8 creates one internally.

I'm not really sure to proceed, but I would suggest to hold off on this V8 upgrade for now and ask the V8 team to reconsider the apparent directional change and reintroduce a hard dependency on TLS.

I've opened a ticket here: https://bugs.chromium.org/p/v8/issues/detail?id=11287, with the risk that it gets buried.

cc @devsnek, any chance you could help us here?

devsnek · 2020-12-31T00:25:06Z

@piscisaureus i updated some tags and added a cc to the bug, can't do much else :)

piscisaureus · 2020-12-31T00:51:50Z

@piscisaureus i updated some tags and added a cc to the bug, can't do much else :)

This is already quite helpful, thanks!

bnoordhuis · 2020-12-31T15:05:17Z

I opened a CL to make the isolate an explicit parameter: https://chromium-review.googlesource.com/c/v8/v8/+/2608209

This floats https://chromium-review.googlesource.com/c/v8/v8/+/2608209 because it just missed the merge window for 8.9.

bnoordhuis · 2021-01-15T12:08:17Z

Okay, the CL landed but just in time to miss the 8.9 merge window. I've switched to my fork of V8 - this PR now uses branch-heads/8.9 + the additional commit from the CL.

(I can also clone v8/v8 to denoland/v8 and use that instead, of course. Using my own fork was less work but no strong preference.)

@piscisaureus PTAL

bnoordhuis commented Dec 28, 2020

View reviewed changes

src/binding.cc Show resolved Hide resolved

bnoordhuis commented Dec 28, 2020

View reviewed changes

bnoordhuis requested a review from piscisaureus December 28, 2020 18:25

bartlomieju reviewed Dec 28, 2020

View reviewed changes

piscisaureus reviewed Dec 30, 2020

View reviewed changes

bartlomieju mentioned this pull request Jan 6, 2021

json modules jstime/jstime#85

Open

bnoordhuis force-pushed the upgrade-v8 branch from 5ae2f5b to fab166e Compare January 12, 2021 12:37

Upgrade V8 to 8.9.255.3

e717b17

This floats https://chromium-review.googlesource.com/c/v8/v8/+/2608209 because it just missed the merge window for 8.9.

bnoordhuis force-pushed the upgrade-v8 branch from fab166e to e717b17 Compare January 15, 2021 12:04

bnoordhuis changed the title ~~Upgrade V8 to 8.9.192~~ Upgrade V8 to 8.9.255.3 Jan 15, 2021

piscisaureus approved these changes Jan 18, 2021

View reviewed changes

bnoordhuis merged commit 0d093a0 into denoland:master Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade V8 to 8.9.255.3 #567

Upgrade V8 to 8.9.255.3 #567

bnoordhuis commented Dec 28, 2020

bnoordhuis left a comment

bnoordhuis Dec 28, 2020

piscisaureus commented Dec 28, 2020

bartlomieju left a comment

bnoordhuis commented Dec 29, 2020

piscisaureus Dec 30, 2020

piscisaureus Dec 30, 2020

bnoordhuis Dec 30, 2020 •

edited

Loading

piscisaureus Jan 1, 2021 •

edited

Loading

bnoordhuis Jan 1, 2021

piscisaureus Dec 30, 2020

bnoordhuis commented Dec 30, 2020 •

edited by piscisaureus

Loading

piscisaureus commented Dec 31, 2020

devsnek commented Dec 31, 2020

piscisaureus commented Dec 31, 2020

bnoordhuis commented Dec 31, 2020

bnoordhuis commented Jan 15, 2021

Upgrade V8 to 8.9.255.3 #567

Upgrade V8 to 8.9.255.3 #567

Conversation

bnoordhuis commented Dec 28, 2020

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Dec 28, 2020

Choose a reason for hiding this comment

piscisaureus commented Dec 28, 2020

bartlomieju left a comment

Choose a reason for hiding this comment

bnoordhuis commented Dec 29, 2020

piscisaureus Dec 30, 2020

Choose a reason for hiding this comment

piscisaureus Dec 30, 2020

Choose a reason for hiding this comment

bnoordhuis Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

piscisaureus Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

bnoordhuis Jan 1, 2021

Choose a reason for hiding this comment

piscisaureus Dec 30, 2020

Choose a reason for hiding this comment

bnoordhuis commented Dec 30, 2020 • edited by piscisaureus Loading

piscisaureus commented Dec 31, 2020

devsnek commented Dec 31, 2020

piscisaureus commented Dec 31, 2020

bnoordhuis commented Dec 31, 2020

bnoordhuis commented Jan 15, 2021

bnoordhuis Dec 30, 2020 •

edited

Loading

piscisaureus Jan 1, 2021 •

edited

Loading

bnoordhuis commented Dec 30, 2020 •

edited by piscisaureus

Loading