Skip to content
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

V8 v11 prep #1794

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

V8 v11 prep #1794

wants to merge 23 commits into from

Conversation

edusperoni
Copy link
Collaborator

This is the prep work for v8 version 11+. Creating the PR so we track it more easily

ptomato and others added 23 commits September 22, 2023 18:28
The Java object ID is an integer returned from Java, so it should be typed
as jint. jint is int32_t, so using uint32_t is incorrect here. Using int
is probably OK, but change it anyway for consistency.
Shadowing the outer jsInfo variable looks like a bug and causes a compiler
warning. But it actually is not a bug, because the earlier
GetJSInstanceInfo() call returns the JSInstanceInfo* pointer, whereas we
need the v8::External to put into the internal field. It's easier to reuse
the same External than to create a new one.
These may be left over from previous refactorings. There are some methods
of ObjectManager that are never used, some pieces of data that are stored
but never read, and some fields that are never stored in the first place.
BREAKING CHANGE: The "markingMode" configuration option in package.json
no longer has any effect. It is always set to "none", which was already
the default. "markingMode": "full" will be ignored.

BREAKING CHANGE: The `__markingMode` global variable no longer exists.

Removing support for full marking mode allows removing a big chunk of GC
finalizer code, which would otherwise need to be ported to CPPGC.
This is always set to true for SDK level 16 and later; and we require
level 17 to build. So it doesn't need to be configurable.
Remove the ObjectManager::SetInstanceIsolate method. It can be considered
part of initializing the ObjectManager after the Isolate is created.
Previously, both SetInstanceIsolate and Init were called with the newly
created Isolate in Runtime::PrepareV8Runtime, but we don't need to have a
2-stage init process for the ObjectManager.
Combine the ObjectManager::Init() method into the ObjectManager
constructor. This way, we don't have to worry about the isolate and things
that depend on it being null. Give Runtime a unique_ptr<ObjectManager>
member.

I'm planning to use this in the upcoming CPPGC port, where I'll be
defining more function templates in the constructor.
These methods don't use the instance pointer, so they can be static - at
least according to Android Studio's static analysis.
Previously we stored a function template in ObjectManager, with which a
constructor function could be created, which would return an empty wrapper
object. Using an object template should achieve the same effect, but
without a call back into JS, so slightly more efficient.
The previous cmake code accidentally set the INTERFACE_INCLUDE_DIRECTORIES
property to the string "NATIVES_BLOB_INCLUDE_DIRECTORIES". We actually
want the value of the variable. (I don't think this affects anything in
the current build, but it complains that INTERFACE_INCLUDE_DIRECTORIES
contains a relative path if you try to link anything else against
libNativeScript.so.)
In the case where a Java exception is thrown from a JNI call, wrapped into
a C++ NativeScriptException, and caught by C++ code that re-throws it to
Java, we'd get a crash.

(Admittedly, this does not happen very much. Initializing the
ObjectManager is one of the few places where it can happen.)
Running the worker tests would occasionally hit a crash where the keywords
set contained corrupt data. I believe this is because the keywords.empty()
check was being hit in two different threads. We can supply the keywords
in an initializer list so that the set is fully populated during static
initialization.
Unlikely, but the ArgConverter calls at the beginning of Runtime::Init()
could theoretically throw a Java exception, in which case they'd be
wrapped into a C++ NativeScriptException and then re-thrown to Java. For
that to work, NativeScriptException has to be initialized.
Particularly in NativeScriptException::ReThrowToJava(), we may need to
call GetClassName before the ObjectManager is initialized. This would
cause a crash when handling exceptions early in the NativeScript runtime's
initialization process.

Luckily, GetClassName doesn't actually need to be part of ObjectManager,
it can be moved to JEnv instead.
Giving a wrong path to DexFactory can result in folders unexpectedly being
wiped. Add a safety check to DexFactory.purgeDexesByThumb() to make sure
we are only deleting files with the expected extensions.
See v8/v8@b38bf5b0. The creation context may be
null for WASM objects, so the old API is deprecated in favour of
GetCreationContext() which returns a MaybeLocal<Context>. However, we are
not handling any WASM objects here, so we can use the version of the API
that asserts that we do have a creation context.
In 11.x, it's no longer allowed to change the flags after V8 has been
initialized.

We can also avoid storing the string in Constants::V8_STARTUP_FLAGS, as
it's no longer used anywhere else in the code base.
For the CPPGC port we'll need to use a cppgc::DefaultPlatform instead of
the v8::platform::NewDefaultPlatform(). It's also harmless to use it
already even if we don't use CPPGC yet.
These sources are already compiled into the V8 artifacts, and are not
compiled as part of the runtime.
This line shadows a v8::Context already present in the outer scope, so
it's not necessary.
…cient

Two changes suggested by Android Studio. We avoid copying the string
argument, and we check quicker whether the map is empty without having to
count all the entries.
JSWrapperConstructorCallback isn't used anywhere.
Noticed while fixing static initialization of keywords set, see next
commit.
@cla-bot cla-bot bot added the cla: yes label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants