V8 v11 prep #1794
Draft
V8 v11 prep #1794
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
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.
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.
This is the prep work for v8 version 11+. Creating the PR so we track it more easily