Optimize JSON #4077

MikeHolman · 2017-10-26T22:07:00Z

This change represents a major overhaul to JSON.stringify.

The new architecture works as follows:

We first calculate the length of the string we need and create a side data structure with the values we will need, but do not actually create any concat strings, etc. like we used to do.

If/when the string is needed, we create a buffer using the length and metadata collected in the first pass, filling it element by element.

If the string is passed to JSON.parse, we use our metadata to construct an object without needing to parse the string.

By avoiding creation of all the intermediate concat strings, this makes JSON.stringify much more efficient. In micro-benchmarks I'm seeing JSON.stringify take around 40% less time than today.

And in case the string is actually sent to JSON.parse, that now takes 70% less time.

This improves Speedometer 2 geomean by about 3%. Also improves AcmeAir by about 3%.

digitalinfinity · 2017-10-26T23:54:45Z

lib/JITIDL/JITTypes.h

@@ -77,7 +77,7 @@ typedef unsigned char boolean;
 #define __JITTypes_h__

 // TODO: OOP JIT, how do we make this better?
-const int VTABLE_COUNT = 48;
+const int VTABLE_COUNT = 49;


Huh- didn't know this existed...is there a compile assert or test that exists to remind us to update this? #Resolved

Yeah there is a compile assert #Closed

digitalinfinity · 2017-10-26T23:55:36Z

lib/Runtime/Base/Constants.h

@@ -163,6 +163,10 @@ namespace Js
        static const size_t GlobalCodeLength        = _countof(_u("Global code")) - 1;
        static const size_t EvalCodeLength          = _countof(_u("eval code")) - 1;
        static const size_t UnknownScriptCodeLength = _countof(_u("Unknown script code")) - 1;
+
+        static const charcount_t NullStringLength = _countof(_u("Null")) - 1;


The above lengths are all size_t - since you're touching this code anyway, can you update those to be charcount_t too? #Resolved

digitalinfinity · 2017-10-26T23:56:27Z

lib/Runtime/Language/JavascriptConversion.cpp

@@ -538,27 +538,25 @@ namespace Js
        }

        // If exoticToPrim is not undefined, then
-        if (nullptr != exoticToPrim)
+        Assert(nullptr != exoticToPrim);


Should this be a fail-fast? What guarantees that this is always not null? #Resolved

If this is null then result is nullptr and it's not valid to pass nullptr to GetTypeId (prefast took issue with this due to some of the annotations I added).

If we get to this point, we will always have called JavascriptOperators::GetPropertyReference for varMethod, and we always assign exoticToPrim = varMethod. #Closed

digitalinfinity · 2017-10-27T00:27:48Z

lib/Runtime/Library/LazyJSONString.h

+    Field(JSONProperty*) jsonContent;
+    Field(const char16*) gap;
+
+    DynamicObject* ParseObject(_In_ JSONObject* valueList) const;


Nit: based on what these functions are doing, I'd prefer something like ReconstructVar/ReconstructObject/ReconstructArray for a slight improvement in readability #Resolved

Totally agree. We're not really parsing at all, are we? #Closed

digitalinfinity · 2017-10-27T00:55:12Z

lib/Runtime/Library/LazyJSONString.cpp

+{
+    const uint elementCount = valueList->Count();
+    PropertyIndex requestedInlineSlotCapacity = static_cast<PropertyIndex>(elementCount);
+    DynamicObject* obj = this->GetLibrary()->CreateObject(true, requestedInlineSlotCapacity);


Nit: annotate true with the parameter name for readability #Resolved

digitalinfinity · 2017-10-27T01:17:04Z

lib/Runtime/Library/LazyJSONString.h

+    String
+};
+
+typedef DListCounted<JSONProperty, Recycler> JSONObject;


Does this need to be a dlist? Or would an slist suffice? Best I can tell, we always just iterate these lists from the beginning?

And actually, I wonder why JSONObject and JSONArray need to be separate types- can JSONObject just be represented as JSONArray- they both look like a list of JSONProperties + Count, and you might be able to avoid a bunch of intermediate allocations

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

I think it could be, but problem with our SLists is that we don't have any way to append. Maybe I'm missing something, but seems like you can only push and pop, which would cause the list to be reversed. I could use SList and then reverse it, but I thought that might be more expensive.

I don't think I can know up front how many properties I am going to enumerate for Objects? #Closed

Ah yeah, forgot append semantics were missing in slists. I wonder how expensive doing two passes with the static enumerator is- if its not too expensive, you might get nice locality benefits by precomputing the size and allocating an array instead of a list?

Although if possible, I'd do it heuristically here- if the number of properties is small, allocate an array- if its large, use a list since it would likely be allocating in a bucket that is generally sparse and cause fragmentation

digitalinfinity · 2017-10-27T01:24:43Z

lib/Runtime/Library/LazyJSONString.h

+struct JSONProperty
+{
+    Field(JSONContentType) type;
+    Field(const PropertyRecord*) propertyRecord;


Do we need both the property record and property name? Can we live with just one? By my rough count, this structure is at 17 bytes which is unfortunate since it'll get allocated in the 32 byte bucket, and the wasted space can add up

You're right I can probably remove the PropertyRecord case without any real impact. I was overoptimizing for the Parse scenario.

digitalinfinity · 2017-10-27T01:39:48Z

lib/Runtime/Library/LazyJSONString.cpp

+LazyJSONString::Parse() const
+{
+    // If the gap is a non-space character, then parsing will be non-trivial transformation
+    for (charcount_t i = 0; i < this->gapLength; ++i)


I understand the reason behind this check but I have a couple of questions:

If the gap is a non-space character, will JSON.parse ever succeed? If not, can we either throw a straight up syntax error, or maybe cache some position/token info and then throw?

If the check here fails, rather than just return null, I would eagerly serialize the string here and throw away the metadata since that's no longer needed (actually, GetSz should probably throw away the intermediate metadata anyway since we dont need to extend the lifetime of those objects). We could even just move the check to the constructor (although delay serialization till Parse is called)

I was thinking you might be able to have something perverse like a gap of {"a":"WHY"}, and so if you do serialize {b:"Because I can!"} you get serialized string of

{ {"a":"WHY"},{"b": "Because I can!"} }

2a. I considered throwing away the metadata if this fails but was lazy and thought it wouldn't happen in practice.

2b. I was wary of throwing away the metadata in case people call parse multiple times on the same string (or need the string and then later call parse on it). But could be that isn't a realistic concern. #Closed

Yuck at 1. For 2, its probably good to release the metadata as soon as we know they're useless so that we don't waste memory. Also, would be nice to have a AutoPtr setting a flag indicating whether the complex parse failed or not and cache that- if it failed, we can throw the syntax error without having to retry the parse

I feel like failing here would be so rare that storing any additional data about it is a waste of memory.

However, I will throw away the metadata after flattening in case of complex gap, because that seems like a much more realistic scenario.

digitalinfinity · 2017-10-27T01:59:51Z

lib/Runtime/Library/JSONStringifier.h

+    bool Has(_In_ RecyclableObject* object)
+    {
+        JSONObjectStack* stack = this;
+        while (stack != nullptr)


Nit: Use list iterator macros? #Pending

I don't have a real list, so not sure what macros you mean?

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

digitalinfinity · 2017-10-27T02:02:31Z

lib/Runtime/Library/JSONStringBuilder.h

+class JSONStringBuilder
+{
+private:
+    ScriptContext* scriptContext;


Nit: all of these fields are consts? like char16* const buffer etc? #Pending

i'm filling the buffer, so that one isn't really a const. i can change some of these to be const, but tbh i find const members to be more effort then they are worth

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

MSLaguana · 2017-10-27T03:01:18Z

lib/Runtime/Library/JSONStringBuilder.cpp

+    this->AppendJSONPropertyString(this->jsonContent);
+    // Null terminate the string
+    *this->currentLocation = _u('\0');
+    Assert(this->currentLocation == buffer + bufferLength - 1);


Is bufferLength only ever used in this assert? Should it be covered by an #ifdef DEBUG in that case?

MSLaguana · 2017-10-27T03:21:08Z

lib/Runtime/Library/JSONStringifier.cpp

+            this->SetStringGap(JavascriptString::FromVar(space));
+            break;
+        case TypeIds_StringObject:
+            this->SetStringGap(JavascriptConversion::ToString(space, this->scriptContext));


Is this correct? Some experimentation seems to suggest that non-string non-number values here have no effect. #Closed

Assuming this corresponds to the third parameter of JSON.stringify(obj, replacer, space) #Closed

Oh nevermind; I misread this as the case for Object, not StringObject. #Closed

MSLaguana · 2017-10-27T03:51:53Z

lib/Runtime/Library/LazyJSONString.cpp

+const char16*
+LazyJSONString::GetSz()
+{
+    const charcount_t allocSize = SafeSzSize();


Should this also check for whether m_pszValue is already set from a previous call to this function, and just return that to avoid re-serializing repeatedly? Or does that already happen somehow?

I don't think GetSz() is generally called, and GetString() does this check, but there might be some cases where this could happen so I'll add in this check.

MSLaguana · 2017-10-27T16:22:52Z

lib/Runtime/Library/JSONStringifier.cpp

+    Var toJSON = nullptr;
+    PolymorphicInlineCache* cache = this->scriptContext->Cache()->toJSONCache;
+    PropertyValueInfo info;
+    PropertyValueInfo::SetCacheInfo(&info, nullptr, cache, false);


It looks like with your annotation changes SetCacheInfo expects a non-null second parameter, so prefast is complaining #Resolved

MSLaguana · 2017-10-27T16:24:59Z

lib/Runtime/Library/JSONStringifier.cpp

+    }
+    if (value == nullptr)
+    {
+        Assert(holder != nullptr);


I think this might need to be an AnalysisAssert since prefast is complaining on the next line that holder might be null. #Resolved

MSLaguana · 2017-10-27T16:36:54Z

lib/Runtime/Library/LazyJSONString.h

+        Field(JSONArray*) arr;
+    };
+
+    JSONProperty()


Looks like clang is complaining about a lack of a copy constructor for JSONProperty which is needed for its use in a DList

Specifically because of const-ness: candidate constructor (the implicit copy constructor) not viable: 1st argument ('const Js::JSONProperty') would lose const qualifier

obastemur · 2017-10-30T19:49:03Z

@MikeHolman did you look at / do you remember the change in memory consumption ?

MikeHolman · 2017-10-31T03:01:22Z

@obastemur Under what scenario are you interested? I didn't do that much memory testing, but in cases where JSON string is a temporary object it doesn't seem to make much difference in memory.

obastemur · 2017-10-31T13:29:14Z

@MikeHolman I didn't test it yet but the description says 3% acme-air win. Curious if number of allocations is the reason. max-peek-memory might tell.

MikeHolman · 2017-11-01T01:25:51Z

@digitalinfinity I think you're right that we can do better than SList in some cases for objects, but maybe that is worth saving for a future checkin?

MSLaguana · 2017-11-03T16:45:37Z

lib/Common/DataStructures/DList.h

@@ -594,6 +594,17 @@ class DListCounted : public DList<TData, TAllocator, RealCount>
    }  \
 }

+#define FOREACH_DLISTCOUNTED_ENTRY(T, alloc, data, list) \


Looks like this isn't used any more now that you switched to SLISTs

…after flattening

Merge pull request #4077 from MikeHolman:lazyjson This change represents a major overhaul to JSON.stringify. The new architecture works as follows: We first calculate the length of the string we need and create a side data structure with the values we will need, but do not actually create any concat strings, etc. like we used to do. If/when the string is needed, we create a buffer using the length and metadata collected in the first pass, filling it element by element. If the string is passed to JSON.parse, we use our metadata to construct an object without needing to parse the string. By avoiding creation of all the intermediate concat strings, this makes JSON.stringify much more efficient. In micro-benchmarks I'm seeing JSON.stringify take around 40% less time than today. And in case the string is actually sent to JSON.parse, that now takes 70% less time. This improves Speedometer 2 geomean by about 3%. Also improves AcmeAir by about 3%.

MikeHolman requested review from obastemur, rajatd and MSLaguana October 26, 2017 22:08

MikeHolman force-pushed the lazyjson branch 3 times, most recently from f10138e to ba073f6 Compare October 26, 2017 22:40

digitalinfinity reviewed Oct 26, 2017

View reviewed changes

digitalinfinity reviewed Oct 27, 2017

View reviewed changes

MSLaguana reviewed Oct 27, 2017

View reviewed changes

MikeHolman force-pushed the lazyjson branch 2 times, most recently from b25030c to 274a1e9 Compare October 31, 2017 02:58

MikeHolman force-pushed the lazyjson branch 3 times, most recently from 0222743 to e3870e2 Compare October 31, 2017 03:08

MikeHolman force-pushed the lazyjson branch 4 times, most recently from bf47771 to dfd0d38 Compare November 1, 2017 00:45

MikeHolman force-pushed the lazyjson branch 2 times, most recently from 9d12daf to 8a4074f Compare November 3, 2017 14:58

MSLaguana reviewed Nov 3, 2017

View reviewed changes

MSLaguana approved these changes Nov 3, 2017

View reviewed changes

MikeHolman added 4 commits November 3, 2017 13:32

Optimize JSON

93aac7e

Address CR comments

8006165

make security checks when flattening buffer, always discard metadata …

bc1d349

…after flattening

revert dlist change

80f5a23

MikeHolman force-pushed the lazyjson branch from 8a4074f to 80f5a23 Compare November 3, 2017 20:33

chakrabot merged commit 80f5a23 into chakra-core:master Nov 3, 2017

rajatd mentioned this pull request Nov 3, 2017

JavascriptOperators::GetTypeId overload with parameter type RecyclableObject* #4113

Closed

Optimize JSON #4077

Optimize JSON #4077

Conversation

MikeHolman commented Oct 26, 2017 • edited Loading

digitalinfinity Oct 26, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

MikeHolman Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

digitalinfinity Oct 26, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

digitalinfinity Oct 26, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

MikeHolman Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

digitalinfinity Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

MikeHolman Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

digitalinfinity Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHolman Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHolman Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalinfinity Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalinfinity Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

MSLaguana Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

MSLaguana Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHolman Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

MSLaguana Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

MSLaguana Oct 27, 2017 • edited by MikeHolman Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur commented Oct 30, 2017

MikeHolman commented Oct 31, 2017

obastemur commented Oct 31, 2017

MikeHolman commented Nov 1, 2017

Choose a reason for hiding this comment

MikeHolman commented Oct 26, 2017 •

edited

Loading

digitalinfinity Oct 26, 2017 •

edited by MikeHolman

Loading

MikeHolman Oct 27, 2017 •

edited

Loading

digitalinfinity Oct 26, 2017 •

edited by MikeHolman

Loading

digitalinfinity Oct 26, 2017 •

edited by MikeHolman

Loading

MikeHolman Oct 27, 2017 •

edited

Loading

digitalinfinity Oct 27, 2017 •

edited by MikeHolman

Loading

MikeHolman Oct 27, 2017 •

edited

Loading

digitalinfinity Oct 27, 2017 •

edited by MikeHolman

Loading

MikeHolman Oct 27, 2017 •

edited

Loading

MikeHolman Oct 27, 2017 •

edited

Loading

digitalinfinity Oct 27, 2017 •

edited by MikeHolman

Loading

digitalinfinity Oct 27, 2017 •

edited by MikeHolman

Loading

MSLaguana Oct 27, 2017 •

edited by MikeHolman

Loading

MSLaguana Oct 27, 2017 •

edited by MikeHolman

Loading

MSLaguana Oct 27, 2017 •

edited by MikeHolman

Loading

MikeHolman Oct 28, 2017 •

edited

Loading

MSLaguana Oct 27, 2017 •

edited by MikeHolman

Loading

MSLaguana Oct 27, 2017 •

edited by MikeHolman

Loading