perf: Improve JSON.stringify performance #3751

obastemur · 2017-09-18T04:22:03Z

Kraken/json-stringify-* perf ~25% better. Acme Air - LTO gain ~1.5%

PR Details:

Improve replacer != function && !HasObjectArray case. (most common use of JSON.stringify)
Add JSON.stringify test cases for ObjectArray, toJSON, and replacer function
IsNumericPropertyId: fast path for internal properties
use requestContext instead of instance->scriptContext
use wmemcpy instead of memcpy

digitalinfinity · 2017-09-18T16:41:26Z

lib/Runtime/Library/JSON.cpp

        // We should look only into object's own properties here. When an object is serialized, only the own properties are considered,
        // the prototype chain is not considered. However, the property names can be selected via an array replacer. In this case
        // ES5 spec doesn't say the property has to own property or even to be enumerable. So, properties from the prototype, or non enum properties,
        // can end up being serialized. Well, that is the ES5 spec word.
        //if(!Js::RecyclableObject::FromVar(holder)->GetType()->GetProperty(holder, keyId, &value))

-        if (VirtualTableInfo<Js::PropertyString>::HasVirtualTable(key))
+        if (value == nullptr)


Just to make sure I understand this change- value is not null only for the stringification of the initial object, right? That is, from the call from Line 328 ( result = stringifySession.Str(scriptContext->GetLibrary()->GetEmptyString(), propertyId, wrapper, value);)?

See Str definition on JSON.h file. value = nullptr default argument. Some of the consumers of Str knows the value hence no need to lookup for it once again

digitalinfinity · 2017-09-18T16:51:53Z

lib/Runtime/Library/JSON.cpp

+    {
+        Js::Var toJSON = nullptr;
+        Js::RecyclableObject* object = Js::RecyclableObject::FromVar(*value);
+        while (typeId != Js::TypeIds_Null)


Instead of doing the prototype walk yourself, why not just call Js::JavascriptOperators::GetProperty here? LTO should take care of optimizing this here, and if the concern is GetProperty does more than we want it to (eg. update the property caches), consider adding a new template parameter to GetProperty_Internal to not update the caches. It's probably better to have property getting centralized to avoid edge cases (eg. what happens if TypeFlagMask_SkipsPrototype is set etc)

Well, I consider this more than a loop hanging around. The logic here is defined underGet_ToJSON. It is probably the second hottest place in JSON.stringify and specifics are clear enough. GetPropertyQuery is a centralized interface no?

What about TypeFlagMask_SkipsPrototype? What is the expected behavior if object is a proxy? etc.

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

What about dealing with TypeFlagMask_SkipsPrototype? What is the expected behavior with Proxies?

Not sure what is the problem there? Can you please elaborate a case where it fails?

In JavascriptOperators::GetProperty_Internal, they explicit break out of the loop if object->SkipsPrototype() is true. I don't quite understand when that flag is used but it looks like its in ChakraFull, so the DOMs type system relies on it. I don't have a ready case that fails but my point is that these are things that JavascriptOperators::GetProperty_Internal that takes into account that you'll need to keep in mind if you're going to reimplement it here, which is why centralizing on JavascriptOperators::GetProperty_Internal is not a bad idea.

Re proxies, again, don't have a failing case- it was more a question of what is the expected behavior.

@digitalinfinity this particular Get_ToJSON, we know it's an object. So, no need for redundant is number, is that or this check. Also we skip cache as well as other centralized stuff we must check for GetProperty_Internal.

Lets say we ended up with version of GetProperty_Internal that we don't do any of these given above. It could be hard to decide on any place that whether we should use that version ever or not.

I didn't want to rule caching out straight and implement GetProperty_InternalWithoutCache, because at this moment, I'm not sure where that cache is beneficial and where it's not. If you prefer, we might also add a TODO note here but I'm also not sure what that could be.

That logic makes sense for number etc but the SkipsPrototype case I mentioned is an edge specific object case- should that be handled here or not?

And I'd still love to understand what the expected behavior here should be for proxies (not saying there's an issue, just want to understand if proxy traps could be invoked as a part of this, in which case there could be other issues).

That logic makes sense for number etc but the SkipsPrototype case I mentioned is an edge specific object case- should that be handled here or not?

I had went through TypeFlagMask_SkipsPrototype and couldn't see any reason why we check it for toJSON. That object doesn't have toJSON in it's body. Well, IMHO we should disable that check for ChakraCore for good. No?

About Proxy; I don't see any reason (on code) why this wouldn't work. There is no proxy trap problem. If the check for HasSpecialType returns true and type was proxy, well we end up doing the same call we do here. However, if the type is null? or other specialized type (which there is no such a thing exist), well they are not the problem of Get_toJSON anyways.

digitalinfinity · 2017-09-18T17:00:48Z

lib/Runtime/Library/JSON.cpp

        Js::Var undefined = scriptContext->GetLibrary()->GetUndefined();
+        Js::TypeId id = Js::JavascriptOperators::GetTypeId(value);

        //check and apply 'toJSON' filter
        if (Js::JavascriptOperators::IsJsNativeObject(value) || (Js::JavascriptOperators::IsObject(value)))


Both these functions call GetTypeId- is it worth creating a new function IsNativeOrUserObject which does the following:
Check if Var is recycler aligned- if not, returns false
Check if the type id is either Native object- if so return true
Check if type id > lastPrimitiveType- return true

Not sure if it would make a huge difference but if this path is hot, it might be noticeable

@digitalinfinity this is one of the two places we use Js::JavascriptOperators::IsJsNativeObject (the one under this function) hence I don't expect perf diff under especially LTO. However, I really liked the idea and will add the interface.

akroshg · 2017-09-18T17:28:53Z

lib/Runtime/Base/ThreadContext.cpp

@@ -1230,6 +1230,8 @@ void ThreadContext::AddBuiltInPropertyRecord(const Js::PropertyRecord *propertyR

 BOOL ThreadContext::IsNumericPropertyId(Js::PropertyId propertyId, uint32* value)
 {
+    if (Js::IsInternalPropertyId(propertyId)) return false;


nit : please have brace even for a single statement body.
Also curious why this function's signature has BOOL even though we return bool (true/false).

akroshg · 2017-09-18T17:37:16Z

lib/Runtime/Library/JSON.cpp

            {
-                return scriptContext->GetLibrary()->GetUndefined();
+                Js::Var values[2];
+                Js::Arguments args(0, values);


nit: why not pass 2, instead of 0 here?

akroshg · 2017-09-18T17:40:47Z

test/JSON/replacerFunction.js

+//-------------------------------------------------------------------------------------------------------
+
+
+var TEST = function(a, b) {


curious why not use the unittestframework here?

script load stuff requires working path to be the script's folder. These extra 4 lines make my life more easier.

RL should take care of that for you

On Windows indeed. ?

It doesn't on xplat? I would expect to see a lot more broken.. What changes would we need to make to have a better experience?

UnitTestFramework has a lot of advantages including a uniform way for us to see errors. Generally the only tests that don't use it are ones that need to run at global scope or syntax errors that have different behavior in function scoped evals.

I would expect to see a lot more broken..

No, because runtests.py sets the path for javascript file's location.

What changes would we need to make to have a better experience?

Some cross platform file system / path related improvements to ch but I don't see any reason to push this.

Fair enough, but new tests of this format should still be using UnitTestFramework unless they can't.

Surely there are some tests, they use multiple stuff from UnitTestFramework. I don't see any reason to push it on these tests...

Does it hurt readability ? or performance of test case? or dependability? or reliability? ... Well, this version of these tests help me to try easier and JSON is and probably will be a hot topic for some time.

UnitTestFramework has many of the benefits you've listed.

It's silent in RL, but verbose in console.

Exceptions explicitly name the type and error message to help regressions where the message changes but the type doesn't

It helps us group tests together by functionality

It encourages descriptions of what you're expecting and why it's an interesting test

Dynamic tests can generate dynamic descriptions

Asserts are smart about deep comparisons, NaN, etc and provide a centralized location to update with edge cases as the language evolves

It helps when someone unfamiliar with the test needs to quickly grok what went wrong (e.g., when merging code, someone not on our team / familiar with our product)

Other JS engines use our unit test framework to run our tests for coverage

E.g.

assert.areEqual(e3, 3, "First identifier in catch param as pattern is matched and initialized correctly");

Language tests from ES6 onward make extensive use of this framework for the above reasons.

Ultimately it's up to you, but we've had a lot of benefits from using the framework. If it's more than a pushd tests/JSON & ch replacerFunction.js I'd be interested to know.

MSLaguana · 2017-09-18T17:41:07Z

lib/Runtime/Library/JSON.cpp

                            {
-                                //if unsuccessful get propertyId from the string
+                                // if unsuccessful get propertyId from the string


Is this correct? Previously this was under a if (id == Js::Constants::NoProperty) conditional, and so the comment/behaviour made sense as a fallback, but now it always gets the property record.

@MSLaguana there are differences in both cases. In this particular case, I don't know any reason for checking id == Js::Constants::NoProperty.

MSLaguana · 2017-09-18T18:35:03Z

lib/Runtime/Library/JSON.cpp

+                            }
+                        }
+
+                        for (PropertyIndex i = 0; i < propertyCount; i++)


I'm a little confused by what's going on here. Could you please add some comments?

MSLaguana · 2017-09-18T18:49:38Z

test/JSON/replacerFunction.js

+    return v;
+}
+
+var until = (!WScript.Platform || WScript.Platform.BUILD_TYPE == 'Debug') ? 12 : 1290;


What is the point of this line here? why construct a larger case in non-debug builds?

See the changes in test configs. So we don't hide this test behind 'Slow' configuration

tcare · 2017-09-18T18:52:31Z

lib/Runtime/Types/PathTypeHandler.cpp

@@ -223,8 +223,7 @@ namespace Js

        // Check numeric propertyId only if objectArray available
        uint32 indexVal;
-        ScriptContext* scriptContext = instance->GetScriptContext();
-        if (instance->HasObjectArray() && scriptContext->IsNumericPropertyId(propertyId, &indexVal))
+        if (instance->HasObjectArray() && requestContext->IsNumericPropertyId(propertyId, &indexVal))


Why are we using requestContext here?

Good question. Why we shouldn't? It will look through ThreadContext either way. No?

Codeflow seemed to lose my comment, apologies if it's repeated..

In this particular case it doesn't matter. The general convention throughout the codebase is to only use requestContext if you have a specific CrossSite marshalling concern.

In a stable class like this we wouldn't expect this to change except in rare circumstances.

As a thought exercise, if these would produce different results I would still expect the instance's scriptContext to be correct since you are asking the object whether this is a numeric id for it's purposes.

I would still expect the instance's scriptContext to be correct since you are asking the object whether this is a numeric id for it's purposes.

Makes sense. Will roll back the change.

akroshg · 2017-09-18T19:32:45Z

lib/Runtime/Library/JSON.cpp

+                      Js::DynamicObject * dynamicObject = ((Js::DynamicObject*)object);
+                      if (ReplacerFunction != replacerType || !dynamicObject->HasObjectArray())
+                      {
+                        int propertyCount = dynamicObject->GetPropertyCount();


int [](start = 24, length = 3)

GetPropertyCount returns uint32

akroshg · 2017-09-18T19:33:10Z

lib/Runtime/Library/JSON.cpp

                        {
-                             if (id == Js::Constants::NoProperty)
+                            int totalNumPropertyCount = this->GetPropertyCount(object, &enumerator) - propertyCount;
+                            Assert(totalNumPropertyCount > 0);


Assert [](start = 28, length = 6)

failfast ?

yes/no? Sorry, didn't understand?

Assert won't do anything under test/release bits. We can use a failfast to die out if the property count is unexpected.

well, I understood as if it happens in release mode, so the loop below goes into infinite loop, would we fail fast?....

The asserted scenario should not happen (if it's happening, something is off and we must see it on debug mode). If anyone expects it might happen??, then it's easy to replace assert with an if check.

That's the intention of failfast. We might not see this in debug mode because of the lack of coverage compared to in-product coverage, so we shouldn't rely on it to catch this case. This is a common pattern in security issues.

AssertOrFailFast is better to use unless there's a specific way we want to recover here, or the branch has perf implications.

@tcare or anyone else, from the implementation, do you know any tangible reason that we should explicitly check this? Otherwise, well, we could failfast so many unlikely stuff though.

ignore compile time generated file

- Improve `replacer != function && !HasObjectArray` case. (most common use of JSON.stringify) - Add JSON.stringify test cases for ObjectArray, toJSON, and replacer function

obastemur · 2017-09-29T14:38:34Z

@akroshg anything else?

akroshg · 2017-09-29T17:00:30Z

LGTM

Merge pull request #3751 from obastemur:fjs Kraken/json-stringify-* perf ~25% better. Acme Air - LTO gain ~1.5% PR Details: - Improve `replacer != function && !HasObjectArray` case. (most common use of JSON.stringify) - Add JSON.stringify test cases for ObjectArray, toJSON, and replacer function - IsNumericPropertyId: fast path for internal properties - use requestContext instead of instance->scriptContext - use wmemcpy instead of memcpy

…rformance Merge pull request #3751 from obastemur:fjs Kraken/json-stringify-* perf ~25% better. Acme Air - LTO gain ~1.5% PR Details: - Improve `replacer != function && !HasObjectArray` case. (most common use of JSON.stringify) - Add JSON.stringify test cases for ObjectArray, toJSON, and replacer function - IsNumericPropertyId: fast path for internal properties - use requestContext instead of instance->scriptContext - use wmemcpy instead of memcpy

msftclas added the cla-already-signed label Sep 18, 2017

obastemur force-pushed the fjs branch 3 times, most recently from a8908af to 304a2f2 Compare September 18, 2017 04:54

digitalinfinity reviewed Sep 18, 2017

View reviewed changes

akroshg reviewed Sep 18, 2017

View reviewed changes

MSLaguana reviewed Sep 18, 2017

View reviewed changes

tcare reviewed Sep 18, 2017

View reviewed changes

akroshg reviewed Sep 18, 2017

View reviewed changes

msftgits removed the cla-already-signed label Sep 26, 2017

obastemur added 5 commits September 29, 2017 15:57

use wmemcpy instead of memcpy

282853e

use requestContext instead of instance->scriptContext

3272c38

IsNumericPropertyId: fast path for internal properties

32538f8

ignore lib/wabt/built/config.h

1289274

ignore compile time generated file

Add missing wchar.h header to use wmemcpy_s on Windows

9ecd10d

obastemur force-pushed the fjs branch 2 times, most recently from 93d4dec to e77b514 Compare September 29, 2017 14:02

JSON.stringify: improve performance

f759841

- Improve `replacer != function && !HasObjectArray` case. (most common use of JSON.stringify) - Add JSON.stringify test cases for ObjectArray, toJSON, and replacer function

obastemur force-pushed the fjs branch from e77b514 to f759841 Compare September 29, 2017 14:13

chakrabot merged commit f759841 into chakra-core:release/1.7 Sep 29, 2017

		//-------------------------------------------------------------------------------------------------------


		var TEST = function(a, b) {

perf: Improve JSON.stringify performance #3751

perf: Improve JSON.stringify performance #3751

Uh oh!

Conversation

obastemur commented Sep 18, 2017

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!

obastemur Sep 18, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

obastemur Sep 18, 2017 • 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!

tcare Sep 19, 2017 • 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!

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!

obastemur Sep 18, 2017 •

edited

Loading

obastemur Sep 18, 2017 •

edited

Loading

tcare Sep 19, 2017 •

edited

Loading