Move isCachedUsingEpoch to HiddenClass#1939
Open
avp wants to merge 16 commits intofacebook:static_hfrom
Open
Conversation
Summary: Add a `CELL_JSOBJECT_NAME` macro that allows us to perform actions only on `JSObject` kinds. This is useful in the next diff in which we introduce fields on `Runtime` for each `JSObject` CellKind. Differential Revision: D82168640
Summary: In order to more easily handle putting the parent pointer in the HC, we want to separate the root classes by their `CellKind` instead of only by the number of slots they have. To that end, we add `HiddenClass` fields to `Runtime` called `classJSObject`, `classJSArray`, etc. (see RuntimeHermesValueFields.def) The "reserved slot" classes used for `DecoratedObject` and `NativeFunction` are kept around, but only for 1 additional slot, because that's easiest. ## GlobalObject We have to update `GlobalObject.cpp` to fill these in as we create the global object and all these well-known objects, prototypes, and classes. This requires instantiating things in the correct order, so that's done manually, along with an assert at the end that all the `class` fields have been filled in properly. ## Creation The `JSObject` subclasses required some `create()` changes in order to make things work. In the default case, we don't take a parent pointer at all, and simply use the default for the CellKind. However, in some cases the parent pointer is required. Certain global object properties have non-standard parents. It's possible to extend things like `JSRegExp` so that requires a custom parent as well. In order to avoid calling `getHiddenClassForPrototype` in every `create()` call, we only call the overload that takes `create()` if the parent is not the standard for the CellKind. The JSLib constructors have been updated accordingly. I've also taken the opportunity to delete a few `create` functions that weren't ever called (to at least attempt to simplify). Creation still uses `getHiddenClassForPrototype` to allow for easily slotting in the code for storing the parent in the `HiddenClass` in the future diff(s). ## Lazy Objects Lazy objects still start with their own special `HiddenClass`es to avoid cache collisions with objects that have real properties all populated. These are retrieved from `getLazyHiddenClassForPrototype`. The `defineLazyProperties` logic had to be updated to use the correct parent/kind because that was never used before, but that's a minor change. Differential Revision: D81177776
Summary: Use the shape table to cache the HiddenClass for `NewObjectWithParent`, which will be useful when we have to store the parent on the `HiddenClass`. This instruction is used when creating classes without declared fields. This diff allows us to avoid a lookup and a `HiddenClass` transition when allocating the new object. Differential Revision: D82654773
Summary: When the parent is in the HiddenClass, we'll need to decode the compressed pointer in order to read the parent `GCPointer`. Split this refactor into its own diff for easier reviewing. Differential Revision: D82657375
Summary: This object is created and then stored in the JSRegExp, and its HiddenClass is used to create the resulting mapping object. When we move the parent into the HiddenClass, we need to also ensure the parent stays the same if we're reusing the HiddenClass, so create the object with a null parent during IRGen when calling `initRegexNamedGroups`. Differential Revision: D82849490
Summary: `HiddenClass` needs a way to store transitions of the parent pointer to a new `HiddenClass`. The existing transition table uses `WeakValueMap`, which doesn't support keying off `JSObject`s, because everything is stored in a `DenseMap` (thus on the C++ heap). This diff adds a new `WeakValueObjectMap` which is keyed off `JSObject`. The keys are referenced strongly and stored in an `ArrayStorage`, while the values are stored in a heap-allocated `WeakRef<GCCell>` array. The `WeakRef`s are stored this way to ensure they don't have to be default-constructed, but this means we have to keep track of which ones are actually constructed so we can destroy them in the `WeakValueObjectMap` finalizer, so we use an `allocatedWeakRefs_` bit vector to track that information (we can't access `keys_` in the finalizer due to not having a `Runtime` and potentially running on another thread). The map itself is quadratically probed and expands and shrinks. The empty/deleted values are easily implemented with empty/null HermesValues because the real keys are all `JSObject`. The main issue is what happens if we run out of space to expand the `keys_` storage, given that there's limits imposed by `ArrayStorage`. Since we're about to use this for a transition table, I've set it up to just return `false` if it fails to insert, allowing the caller to clear the map and start over (because this isn't necessarily a failure scenario for our use case). Differential Revision: D82654775
Summary: The `HiddenClass` is cached for a given shape table entry. When we add the parent pointer to the `HiddenClass`, it will mean that we have to check that the parent is also correct when reading from the cache. We also can ensure that the instructions that do NOT allow a custom parent (`NewObjectWithBuffer`) don't need to check the parent for the cached `HiddenClass` by making sure they never share shape table entries with instructions that allow a custom parent. Note that this will NOT eliminate the need to check that the parent is correct when reading the `HiddenClass` cache when the parent is specified, but it will help the cache hit more often. Differential Revision: D82352376
Summary: Add a shape table entry parameter to the CreateThisForNew and CreateThisForSuper bytecode instructions. The entry is used to cache the HiddenClass of objects created during constructor calls. This is necessary for performance reasons. We can avoid repeated HiddenClass lookups and transitions when constructing objects with the 'new' operator, improving the efficiency of object creation in hot paths. Without this change, moving the parent to the HiddenClass was a significant (7-8%) performance regression on certain benchmarks such as octane raytrace. This diff mitigates about half that regression. Differential Revision: D84391003
Summary: In order to store the object's parent in the `HiddenClass` instead of on `JSObject` itself, we need to have a way to store the transitions, which we do with a `WeakValueObjectMap`. This map may hit its capacity limit, so in that case we first try to clear the map and if that doesn't work, we OOM. The map will be allocated when the first parent change happens because the vast majority of `HiddenClass` will not have a parent change transition at all. In order to avoid an unbounded chain of parent changes, we put a hard limit on how many changes can happen in a given chain. If the limit is reached, the class is converted to dictionary mode, where parent changes won't allocate a new `HiddenClass`. We use `JSObject` for all the keys in the parent transition map, so we make a dummy object called `nullParentTransitionObject` to indicate the transition to the `null` parent and use that as the key instead of having to account for null keys in the `WeakValueObjectMap`. When reading cached `HiddenClass` entries from the shape table's cache, we need to also check that the parent is correct in order to have a cache hit. IMPORTANT: This diff currently results in a 3-4% regression on certain benchmarks like octane raytrace because they hit the `JSObject::getParent` function a lot and it now has to go through an extra indirection. Switching it back to `return parent_` does mitigate the regression entirely. NOTE: This diff is set up to easily check for correctness, because it does NOT eliminate the `parent_` pointer on `JSObject` itself, merely duplicates it in `HiddenClass` and asserts that these are the same whenever `JSObject::getParent()` is called. A later diff will remove the `parent_` pointer from `JSObject`. Differential Revision: D82352374
Summary: This is mostly a code deletion. It's likely that these 4 bytes will eventually be taken up by an indexed property storage slot but until then we can use the extra space. Removing the field does trigger a bunch of static assertion failures because we're wasting space if we don't compensate by adding a new slot in internal storage (improving memory consumption). In order to accommodate the existing `additionalSlots` functionality in `NativeFunction` we also add an extra anonymous internal property. NOTE: This does result in a slight performance improvement on certain react benchmarks (up to 3%) due to the extra direct storage slot. Differential Revision: D83193048
Summary: This doesn't take up extra space because we have 2 free bytes. We can use this to cache transitions between HiddenClasses that only modify ClassFlags. Explicitly expand the `ClassFlags` to 16 bits to make it clear what's going on (we're about to use more bits anyway). We'll add an internal property for just modifying class flags once we have functionality that can use it. Differential Revision: D84654980
Summary: We're about to move a lot of relevant flags to the HiddenClass, so start checking it to avoid correctness issues due to frozen objects, etc. We use the unused padding field to ensure we have full control over all the bits in the struct. Differential Revision: D84654983
Summary: These 3 flags are provided at construction, so move them and make it easy to access them. Differential Revision: D84654979
Summary: These flags can be modified on a given object, which now changes the HiddenClass. Add an `updateClassFlags` function to `HiddenClass` which uses a new `InternalProperty` to indicate a flags transition. The transition needs to match both the SymbolID and the new flags in order for the new `HiddenClass` to be used. Differential Revision: D84654982
Summary: This flag can never be changed once the object is created. Differential Revision: D84654985
Summary: Marking an object as cached using the epoch may now allocate a new `HiddenClass`, so some care has to be taken due to the various NoAllocScope uses around this flag. Differential Revision: D84654981
meta-cla
bot
added
the
CLA Signed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary:
Marking an object as cached using the epoch may now allocate a new
HiddenClass, so some care has to be taken due to the variousNoAllocScope uses around this flag.
Differential Revision: D84654981