ObjectManager improvements: unused code removal, consolidation #1769
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.
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.
Description
This PR contains some improvements to ObjectManager that I made while working on the port to CPPGC. It consists mostly of removal of unused features and dead code (the static analysis of Android Studio is really helpful here); the less code there is in ObjectManager, the less has to be ported to work with CPPGC. 😄
Please do double-check the removed unused code to make sure that it is supposed to be unused; it's also possible that something was made dead code by accident, and is actually still supposed to be used somewhere!
Best reviewed commit by commit. Further descriptions of each commit are in the individual commit messages.
Related Pull Requests
None.
Does your pull request have unit tests?
This PR removes the unit tests that were only run in "full" marking mode, because that mode no longer exists. All other functionality should remain unchanged, so no new unit tests added.