Speed up property accesses using Symbols by sethbrenith · Pull Request #4569 · chakra-core/ChakraCore

sethbrenith · 2018-01-17T23:42:26Z

This change puts a polymorphic inline cache on JavascriptSymbol so that it can act like PropertyString and cache information about loads and stores that use it. It also adds a cache mapping property IDs to JavascriptSymbol weak refs in ScriptContext, similar to the existing cache for PropertyStrings, and modifies existing code to not create new JavascriptSymbol instances unless necessary. When lowering a load or store with a likely symbol, we can emit code that is very similar to the fast path for strings.

sethbrenith · 2018-01-17T23:45:41Z

        PolymorphicInlineCache * polymorphicInlineCache;
        FunctionBody * functionBody;
-        PropertyString * propertyString;
+        PropertyRecordUsageCache* propertyRecordUsageCache; // Interior pointer to a Symbol or PropertyString. Okay as long as PropertyValueInfo is stack-allocated.


Interior pointer [](start = 63, length = 16)

This seems moderately creepy; I'm open to other suggestions for how to represent a thing that's either a Symbol or a PropertyString.

What keeps the Symbol/PropertyString alive? You might want either an additional pointer to the DynamicObject that owns the PropertyRecordUsageCache or make this a union of Symbol & PropertyString?

In reply to: 162213674 [](ancestors = 162213674)

I like your idea of keeping an extra pointer to the owning object, thanks! It seems that PropertyValueInfo is always stack-allocated, and you mentioned that stack references are checked for interior pointers, but relying on that behavior feels very brittle.

In reply to: 162227539 [](ancestors = 162227539,162213674)

sethbrenith · 2018-01-17T23:48:23Z

+        {
+            if (!baseOpnd->GetValueType().IsLikelyOptimizedTypedArray())
+            {
+                // If profile data says that it's a typed array - do not generate the symbol fast path as the src. could be a temp and that would cause a bug.


could be a temp and that would cause a bug. [](start = 115, length = 43)

I copied this but don't really know if it's necessary in this case or even still relevant. Comment from change that introduced it is below. I tried removing the typed array checks in the string case above and running typedArrayProfile.js, and I saw no problems.

The problem is that the PropertyString fast path was getting generated within a forin even though the profile data said that the store was on a typed array. Using the profile data, the src would be marked temp as it was a typed array store. At runtime, the property string fast path was actually be successful and store a temp stack allocated javascript number. When the stack allocated number was used, it would cause an AV. I have also added a unit test for this issue.

sethbrenith · 2018-01-17T23:53:09Z

                // the cached property string for this property, if any, does not
                // specify this object's type.
-                scriptContext->InvalidatePropertyStringCache(propId, obj->GetType());
+                scriptContext->InvalidatePropertyStringAndSymbolCaches(propId, obj->GetType());


I still haven't found how to construct a test case where there's actually work to do here. Either the writable data detection bit is not set, the type was replaced with a new type, or the property string was not cached. To test that the cache was being invalidated, I temporarily removed the writable-data-only check.

digitalinfinity · 2018-01-18T01:31:18Z

        {
-            CacheOperators::CachePropertyRead(startingObject, this->object, false, propertyId, false, &info, scriptContext);
-            if ((!(this->flags & EnumeratorFlags::EphemeralReference)) && info.IsStoreFieldCacheEnabled() && info.IsWritable() && ((info.GetFlags() & (InlineCacheGetterFlag | InlineCacheSetterFlag)) == 0))
+            PropertyString* propertyStringAsPropertyString = propertyString == nullptr ? nullptr : PropertyString::TryFromVar(propertyString);


When would this be null if we have a cache and we should be using it?

It looked to me like propertyString could be null if the flags said to fetch Symbol properties, though we may never use this method with those flags.

In reply to: 162228454 [](ancestors = 162228454)

digitalinfinity · 2018-01-18T01:32:00Z


 #include "Library/JavascriptVariantDate.h"
 #include "Library/JavascriptPromise.h"
-#include "Library/JavascriptSymbol.h"


Why was this removed?

I added it to Runtime.h because some new files needed access to it, and then removed it from everywhere else that already includes Runtime.h.

In reply to: 162228544 [](ancestors = 162228544)

digitalinfinity · 2018-01-18T01:39:02Z

+        stElemInlineCache(nullptr),
+        hitRate(0)
+    {
+        // TODO: in future, might be worth putting these inline to avoid extra allocations. PIC copy API needs to be updated to support this though


Actually, given this comment, the solution might be to allocate PropertyRecordUsageCache dynamically rather than statically? If you do that and then put the inline caches inline on it, you end up net-saving one allocation :)

Although, yes, you'd still have to be careful doing that since now your inline caches become interior pointers

In reply to: 162229422 [](ancestors = 162229422)

I'd prefer to leave this alone for now (this comment was just copied over from PropertyString, along with most of this class's implementation).

In reply to: 162229510 [](ancestors = 162229510,162229422)

jackhorton · 2018-01-19T23:56:13Z

If this is for Node, shouldn't it target release/1.9? Also should it get mainlined before 1.9.0/8.9.4 is cut?

digitalinfinity

LGTM although I don't recall what our philosophy is around squashing commits- @dilijev do you remember?

Also, @MikeHolman should review this change too

dilijev · 2018-01-23T02:13:09Z

Squash is best-effort at keeping the history as clean as possible. If there's useful history here for posterity (changes that can be grouped by broad feature or change, or which makes certain changes easier to review), then keep some commits. In general, cleanup and merge commits are not that useful. IMO this PR can be squashed to a single commit on top of the head of the branch.

MikeHolman · 2018-01-23T03:09:51Z

    <ClCompile Include="$(MSBuildThisFileDirectory)JSONStringBuilder.cpp" />
    <ClCompile Include="$(MSBuildThisFileDirectory)JSONStringifier.cpp" />
    <ClCompile Include="$(MSBuildThisFileDirectory)LazyJSONString.cpp" />
+    <ClCompile Include="PropertyRecordUsageCache.cpp" />


nit: for razzle build to work, we need to prepend with $(MSBuildThisFileDirectory)

Great catch! I just let VS do whatever it wanted to here, but it didn't match the correct style.

In reply to: 163133827 [](ancestors = 163133827)

sethbrenith · 2018-01-23T16:14:11Z

Okay, thanks. I'll squash before merging. None of these intermediate states are useful.

In reply to: 359649425 [](ancestors = 359649425)

sethbrenith · 2018-01-23T16:17:04Z

Discussed offline; master is fine for this change.

In reply to: 359122247 [](ancestors = 359122247)

This change puts a polymorphic inline cache on JavascriptSymbol so that it can act like PropertyString and cache information about loads and stores that use it. It also adds a cache mapping property IDs to JavascriptSymbol weak refs in ScriptContext, similar to the existing cache for PropertyStrings, and modifies existing code to not create new JavascriptSymbol instances unless necessary. When lowering a load or store with a likely symbol, we can emit code that is very similar to the fast path for strings.

Merge pull request #4569 from sethbrenith:user/sethb/cache-symbol-ops This change puts a polymorphic inline cache on JavascriptSymbol so that it can act like PropertyString and cache information about loads and stores that use it. It also adds a cache mapping property IDs to JavascriptSymbol weak refs in ScriptContext, similar to the existing cache for PropertyStrings, and modifies existing code to not create new JavascriptSymbol instances unless necessary. When lowering a load or store with a likely symbol, we can emit code that is very similar to the fast path for strings.

sethbrenith requested review from MikeHolman and boingoing January 17, 2018 23:43

sethbrenith commented Jan 17, 2018

View reviewed changes

digitalinfinity reviewed Jan 18, 2018

View reviewed changes

digitalinfinity approved these changes Jan 22, 2018

View reviewed changes

MikeHolman reviewed Jan 23, 2018

View reviewed changes

MikeHolman approved these changes Jan 23, 2018

View reviewed changes

sethbrenith force-pushed the user/sethb/cache-symbol-ops branch from e982e97 to 4b03b4a Compare January 23, 2018 16:36

chakrabot merged commit 4b03b4a into chakra-core:master Jan 23, 2018

Conversation

sethbrenith commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethbrenith Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackhorton commented Jan 19, 2018

Uh oh!

digitalinfinity left a comment

Choose a reason for hiding this comment

Uh oh!

dilijev commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethbrenith commented Jan 23, 2018

Uh oh!

sethbrenith commented Jan 23, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sethbrenith Jan 17, 2018 •

edited

Loading

dilijev commented Jan 23, 2018 •

edited

Loading