Skip to content

Commit

Permalink
Address CR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@MikeHolman
MikeHolman committed Nov 1, 2017
1 parent ba073f6 commit dfd0d38
Show file tree
Hide file tree
Showing 12 changed files with 151 additions and 67 deletions.
17 changes: 8 additions & 9 deletions lib/Runtime/Base/Constants.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,15 +155,14 @@ namespace Js
static const char16 StringReplace[];
static const char16 StringMatch[];

static const size_t AnonymousFunctionLength = _countof(_u("Anonymous function")) - 1;
static const size_t AnonymousLength = _countof(_u("anonymous")) - 1;
static const size_t AnonymousClassLength = _countof(_u("Anonymous class")) - 1;
static const size_t FunctionCodeLength = _countof(_u("Function code")) - 1;
static const size_t GlobalFunctionLength = _countof(_u("glo")) - 1;
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 AnonymousFunctionLength = _countof(_u("Anonymous function")) - 1;
static const charcount_t AnonymousLength = _countof(_u("anonymous")) - 1;
static const charcount_t AnonymousClassLength = _countof(_u("Anonymous class")) - 1;
static const charcount_t FunctionCodeLength = _countof(_u("Function code")) - 1;
static const charcount_t GlobalFunctionLength = _countof(_u("glo")) - 1;
static const charcount_t GlobalCodeLength = _countof(_u("Global code")) - 1;
static const charcount_t EvalCodeLength = _countof(_u("eval code")) - 1;
static const charcount_t UnknownScriptCodeLength = _countof(_u("Unknown script code")) - 1;
static const charcount_t NullStringLength = _countof(_u("Null")) - 1;
static const charcount_t TrueStringLength = _countof(_u("True")) - 1;
static const charcount_t FalseStringLength = _countof(_u("False")) - 1;
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/JSON.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ namespace JSON
{
// Try to reconstruct object based on the data collected during stringify
// In case of semantically important gap, this call may fail and we will need to do real parse
result = lazyString->Parse();
result = lazyString->TryParse();
}
if (result == nullptr)
{
Expand Down
6 changes: 3 additions & 3 deletions lib/Runtime/Library/JSONStringBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ JSONStringBuilder::AppendObjectString(_In_ JSONObject* valueList)
}

bool isFirstMember = true;
FOREACH_DLISTCOUNTED_ENTRY(JSONProperty, Recycler, entry, valueList)
FOREACH_SLISTCOUNTED_ENTRY(JSONObjectProperty, entry, valueList)
{
if (!isFirstMember)
{
Expand All @@ -158,11 +158,11 @@ JSONStringBuilder::AppendObjectString(_In_ JSONObject* valueList)
this->AppendCharacter(_u(' '));
}

this->AppendJSONPropertyString(&entry);
this->AppendJSONPropertyString(&entry.propertyValue);

isFirstMember = false;
}
NEXT_DLISTCOUNTED_ENTRY;
NEXT_SLISTCOUNTED_ENTRY;

if (this->gap != nullptr)
{
Expand Down
36 changes: 24 additions & 12 deletions lib/Runtime/Library/JSONStringifier.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
namespace Js
{

const charcount_t JSONStringifier::MaxGapLength = 10;

JSONStringifier::JSONStringifier(_In_ ScriptContext* scriptContext) :
scriptContext(scriptContext),
replacerFunction(nullptr),
Expand Down Expand Up @@ -108,7 +110,7 @@ JSONStringifier::AddToPropertyList(_In_ Var item, _Inout_ BVSparse<Recycler>* pr
PropertyListElement elem;
elem.propertyName = propertyName;
elem.propertyRecord = propertyRecord;
this->propertyList->Append(elem);
this->propertyList->Push(elem);
}
}
}
Expand Down Expand Up @@ -156,6 +158,8 @@ JSONStringifier::ReadReplacer(_In_opt_ Var replacer)
}
}
}
// PropertyList is an SList, which only has push/pop, so need to reverse list for it to be in order
this->propertyList->Reverse();
}
}
}
Expand Down Expand Up @@ -412,11 +416,12 @@ JSONStringifier::ReadObjectElement(
_In_ JSONObject* jsonObject,
_In_ JSONObjectStack* objectStack)
{
JSONProperty element;
this->ReadProperty(propertyName, obj, &element, nullptr, propertyRecord, objectStack);
JSONObjectProperty prop;
prop.propertyName = propertyName;
this->ReadProperty(propertyName, obj, &prop.propertyValue, nullptr, propertyRecord, objectStack);

// Undefined result is not concatenated
if (element.type != JSONContentType::Undefined)
if (prop.propertyValue.type != JSONContentType::Undefined)
{
// Increase length for the name of the property
this->totalStringLength = UInt32Math::Add(this->totalStringLength, CalculateStringElementLength(propertyName));
Expand All @@ -427,7 +432,7 @@ JSONStringifier::ReadObjectElement(
// If gap is specified, a space is appended
UInt32Math::Inc(this->totalStringLength);
}
jsonObject->Append(element);
jsonObject->Push(prop);
}
}

Expand Down Expand Up @@ -520,11 +525,11 @@ JSONStringifier::ReadObject(_In_ RecyclableObject* obj, _In_ JSONObjectStack* ob
// If we are given a property list, we should only read the listed properties
if (this->propertyList != nullptr)
{
FOREACH_DLIST_ENTRY(PropertyListElement, Recycler, entry, this->propertyList)
FOREACH_SLIST_ENTRY(PropertyListElement, entry, this->propertyList)
{
this->ReadObjectElement(entry.propertyName, entry.propertyRecord, obj, jsonObject, &stack);
}
NEXT_DLIST_ENTRY;
NEXT_SLIST_ENTRY;
}
else if (JavascriptProxy::Is(obj))
{
Expand All @@ -537,6 +542,7 @@ JSONStringifier::ReadObject(_In_ RecyclableObject* obj, _In_ JSONObjectStack* ob
JavascriptStaticEnumerator enumerator;
if (obj->GetEnumerator(&enumerator, EnumeratorFlags::SnapShotSemantics | EnumeratorFlags::EphemeralReference, this->scriptContext))
{
enumerator.GetInitialPropertyCount();
JavascriptString* propertyName = nullptr;
PropertyId nextKey = Constants::NoProperty;
while ((propertyName = enumerator.MoveAndGetNext(nextKey)) != nullptr)
Expand All @@ -551,6 +557,8 @@ JSONStringifier::ReadObject(_In_ RecyclableObject* obj, _In_ JSONObjectStack* ob
}
}
}
// JSONObject is an SList, which only has push/pop, so need to reverse list for it to be in order
jsonObject->Reverse();
const uint propertyCount = jsonObject->Count();

this->CalculateStringifiedLength(propertyCount, stepbackLength);
Expand Down Expand Up @@ -730,7 +738,13 @@ RecheckValue(_In_ Var value, _Out_ RecyclableObject** valueObj, _Out_ bool* isOb
}

void
JSONStringifier::ReadProperty(_In_ JavascriptString* key, _In_opt_ RecyclableObject* holder, _Out_ JSONProperty* prop, _In_opt_ Var value, _In_opt_ const PropertyRecord* propertyRecord, _In_ JSONObjectStack* objectStack)
JSONStringifier::ReadProperty(
_In_ JavascriptString* key,
_In_opt_ RecyclableObject* holder,
_Out_ JSONProperty* prop,
_In_opt_ Var value,
_In_opt_ const PropertyRecord* propertyRecord,
_In_ JSONObjectStack* objectStack)
{
PROBE_STACK(this->scriptContext, Constants::MinStackDefault);
if (propertyRecord == nullptr)
Expand All @@ -739,13 +753,11 @@ JSONStringifier::ReadProperty(_In_ JavascriptString* key, _In_opt_ RecyclableObj
}
if (value == nullptr)
{
Assert(holder != nullptr);
// If we don't have a value, we must have an object from which we can read it
AnalysisAssert(holder != nullptr);
value = this->ReadValue(key, propertyRecord, holder);
}

prop->propertyRecord = propertyRecord;
prop->propertyName = key;

// Save these to avoid having to recheck conditions unless value is modified by ToJSON or a replacer function
RecyclableObject* valueObj = nullptr;
bool isObject = false;
Expand Down
7 changes: 4 additions & 3 deletions lib/Runtime/Library/JSONStringifier.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,15 @@ class JSONStringifier
{
private:
// Spec defined limit on the gap length
static const charcount_t MaxGapLength = 10;
static const charcount_t MaxGapLength;

struct PropertyListElement
{
PropertyRecord const* propertyRecord;
JavascriptString* propertyName;
};

typedef DList<PropertyListElement, Recycler> PropertyList;
typedef SList<PropertyListElement, Recycler> PropertyList;

ScriptContext* scriptContext;
RecyclableObject* replacerFunction;
Expand All @@ -51,7 +51,6 @@ class JSONStringifier
charcount_t gapLength;
char16* gap;

// TODO: refactor
_Ret_notnull_ const PropertyRecord* GetPropertyRecord(_In_ JavascriptString* string);
Var TryConvertPrimitiveObject(_In_ RecyclableObject* value);
Var ToJSON(_In_ JavascriptString* key, _In_ RecyclableObject* valueObject);
Expand Down Expand Up @@ -100,6 +99,8 @@ class JSONStringifier
charcount_t GetGapLength() const { return this->gapLength; }
bool HasReplacerFunction() const { return this->replacerFunction != nullptr; }

bool HasComplexGap() const;

static LazyJSONString* Stringify(_In_ ScriptContext* scriptContext, _In_ Var value, _In_opt_ Var replacer, _In_opt_ Var space);

}; // class JSONStringifier
Expand Down
104 changes: 76 additions & 28 deletions lib/Runtime/Library/LazyJSONString.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,48 +18,83 @@ LazyJSONString::LazyJSONString(_In_ JSONProperty* jsonContent, charcount_t lengt
SetLength(length);
}

bool
LazyJSONString::HasComplexGap() const
{
for (charcount_t i = 0; i < this->gapLength; ++i)
{
switch (this->gap[i])
{
// This is not exhaustive, just a useful subset of semantics preserving characters
case _u(' '):
case _u('\t'):
case _u('\n'):
continue;
default:
return true;
}
}
return false;
}

DynamicObject*
LazyJSONString::ParseObject(_In_ JSONObject* valueList) const
LazyJSONString::ReconstructObject(_In_ JSONObject* valueList) const
{
const uint elementCount = valueList->Count();

// This is just a heuristic, so overflow doesn't matter
PropertyIndex requestedInlineSlotCapacity = static_cast<PropertyIndex>(elementCount);
DynamicObject* obj = this->GetLibrary()->CreateObject(true, requestedInlineSlotCapacity);
FOREACH_DLISTCOUNTED_ENTRY(JSONProperty, Recycler, entry, valueList)

DynamicObject* obj = this->GetLibrary()->CreateObject(
true, // allowObjectHeaderInlining
requestedInlineSlotCapacity);

FOREACH_SLISTCOUNTED_ENTRY(JSONObjectProperty, entry, valueList)
{
Var propertyValue = Parse(&entry);
Var propertyValue = ReconstructVar(&entry.propertyValue);
JavascriptString* propertyName = entry.propertyName;
PropertyString* propertyString = PropertyString::TryFromVar(propertyName);
PropertyValueInfo info;
PropertyString* propertyString = PropertyString::TryFromVar(entry.propertyName);
if (!propertyString || !propertyString->TrySetPropertyFromCache(obj, propertyValue, this->GetScriptContext(), PropertyOperation_None, &info))
{
JavascriptOperators::SetProperty(obj, obj, entry.propertyRecord->GetPropertyId(), propertyValue, &info, this->GetScriptContext());
const PropertyRecord* propertyRecord = nullptr;
if (propertyString)
{
propertyRecord = propertyString->GetPropertyRecord();
}
else
{
this->GetScriptContext()->GetOrAddPropertyRecord(propertyName, &propertyRecord);
}
JavascriptOperators::SetProperty(obj, obj, propertyRecord->GetPropertyId(), propertyValue, &info, this->GetScriptContext());
}
}
NEXT_DLISTCOUNTED_ENTRY;
NEXT_SLISTCOUNTED_ENTRY;
return obj;
}

JavascriptArray*
LazyJSONString::ParseArray(_In_ JSONArray* jsonArray) const
LazyJSONString::ReconstructArray(_In_ JSONArray* jsonArray) const
{
const uint32 length = jsonArray->length;
JavascriptArray* arr = this->GetLibrary()->CreateArrayLiteral(length);
JSONProperty* prop = jsonArray->arr;
for (uint i = 0; i < length; ++i)
{
Var element = Parse(&prop[i]);
Var element = ReconstructVar(&prop[i]);
BOOL result = arr->SetItem(i, element, PropertyOperation_None);
Assert(result); // Setting item in an array we just allocated should always succeed
}
return arr;
}

Var
LazyJSONString::Parse(_In_ JSONProperty* prop) const
LazyJSONString::ReconstructVar(_In_ JSONProperty* prop) const
{
switch (prop->type)
{
case JSONContentType::Array:
return ParseArray(prop->arr);
return ReconstructArray(prop->arr);
case JSONContentType::Null:
return this->GetLibrary()->GetNull();
case JSONContentType::True:
Expand All @@ -71,37 +106,43 @@ LazyJSONString::Parse(_In_ JSONProperty* prop) const
case JSONContentType::String:
return prop->stringValue;
case JSONContentType::Object:
return ParseObject(prop->obj);
return ReconstructObject(prop->obj);
default:
Assume(UNREACHED);
return nullptr;
}
}

Var
LazyJSONString::Parse() const
LazyJSONString::TryParse() 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)
// If we have thrown away our metadata, we won't be able to Parse
if (this->jsonContent == nullptr)
{
switch (this->gap[i])
{
// This is not exhaustive, just a useful subset of semantics preserving characters
case _u(' '):
case _u('\t'):
case _u('\n'):
continue;
default:
return nullptr;
}
return nullptr;
}
return Parse(this->jsonContent);

// If the gap is complex, this means that parse will be a non-trivial transformation,
// so fall back to the real parse
if (this->HasComplexGap())
{
return nullptr;
}

Var result = ReconstructVar(this->jsonContent);

return result;
}

const char16*
LazyJSONString::GetSz()
{
const charcount_t allocSize = SafeSzSize();
if (this->IsFinalized())
{
return this->UnsafeGetBuffer();
}

const charcount_t allocSize = this->SafeSzSize();

Recycler* recycler = GetScriptContext()->GetRecycler();
char16* target = RecyclerNewArrayLeaf(recycler, char16, allocSize);
Expand All @@ -116,7 +157,14 @@ LazyJSONString::GetSz()

builder.Build();

SetBuffer(target);
this->SetBuffer(target);

if (this->HasComplexGap())
{
// If we have a complex gap, there is no reason to keep content around after flattening
this->jsonContent = nullptr;
}

return target;
}

Expand Down
Loading

0 comments on commit dfd0d38

Please sign in to comment.