JSONStack: Improve performance #3465
Conversation
| @@ -37,22 +54,68 @@ namespace JSON | |||
| { | |||
| if (bJsObject) | |||
| { | |||
| return jsObjectStack.Push(data); | |||
| bool result = true; | |||
There was a problem hiding this comment.
Consider refactoring this into an inline function- something like PushObject(data, jsObjectStack, &STACK_VALUE)/PushObject(data, domObjectStack, &DOM_VALUE)
There was a problem hiding this comment.
@digitalinfinity This also came to my mind. Good idea. However, all three functions may end up in similar inline function pairs. Current looked more cleaner (whole file is ~100 lines)
| @@ -15,6 +15,7 @@ namespace JSON | |||
| class JSONStack | |||
| { | |||
| private: | |||
| Js::Var tempValues[2]; | |||
There was a problem hiding this comment.
Not really your change, but more a question- are these references to Vars supposed to be kept alive? I see that this class uses an arena allocator so curious who keeps it alive
There was a problem hiding this comment.
@digitalinfinity not sure about the answer. I added a destructor to make sure they are nullified.
There was a problem hiding this comment.
This is not a problem for JSONStack as it always cleans up the references once stringify is done with inner object/array etc.
lib/Runtime/Library/JSONStack.cpp
Outdated
| @@ -24,10 +33,18 @@ namespace JSON | |||
| { | |||
| if (bJsObject) | |||
| { | |||
| if (STACK_VALUE) | |||
| { | |||
| return (data == STACK_VALUE); | |||
There was a problem hiding this comment.
return (data == STACK_VALUE); [](start = 16, length = 29)
just to guard future changes to this class, can you add assert at appropriate places to check when STACK_VALUE != nullptr, jsobjectStack is always empty and similarly for dom stack?
There was a problem hiding this comment.
@kunalspathak not sure I understood. there is an if(STACK_VALUE) why should I check STACK_VALUE != nullptr inside an Assert?
There was a problem hiding this comment.
I meant to say add assert something like
if(STACK_VALUE)
{
assert(jsObjectStack.Count() == 0);
return (data == STACK_VALUE)
}1% Acme Air LTO gain. Majority of the time, there is at max 1 item on the stacks and SList is too expensive to maintain 1 item in/out during the whole JSON.stringify
obastemur
force-pushed
the
imp_jsonstack
branch
from
ad61f6c
to
2e19ab7
Compare
|
@dotnet-bot test Ubuntu shared_ubuntu_linux_test please |
Merge pull request #3465 from obastemur:imp_jsonstack 1% Acme Air LTO gain. Majority of the time, there is at max 1 item on the stacks and SList too expensive to maintain 1item in/out during the whole JSON.stringify
|
@digitalinfinity @kunalspathak Thanks for the review! |
Merge pull request #3465 from obastemur:imp_jsonstack 1% Acme Air LTO gain. Majority of the time, there is at max 1 item on the stacks and SList too expensive to maintain 1item in/out during the whole JSON.stringify
1% Acme Air LTO gain. Majority of the time, there is at max 1 item on the stacks and SList too expensive to maintain 1item in/out during the whole JSON.stringify