Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize object.assign({}, obj) #4817

Merged
merged 5 commits into from
Mar 21, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/Common/ConfigFlagsList.h
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ PHASE(All)
PHASE(Error)
PHASE(PropertyRecord)
PHASE(TypePathDynamicSize)
PHASE(ObjectCopy)
PHASE(ShareTypesWithAttributes)
PHASE(ShareAccessorTypes)
PHASE(ConditionalCompilation)
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Debug/TTSnapObjects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ namespace TTD
TTDVar* cpyBase = snpObject->VarArray;
if(sHandler->InlineSlotCapacity != 0)
{
Js::Var const* inlineSlots = dynObj->GetInlineSlots_TTD();
Field(Js::Var) const* inlineSlots = dynObj->GetInlineSlots_TTD();

//copy all the properties (if they all fit into the inline slots) otherwise just copy all the inline slot values
uint32 inlineSlotCount = min(sHandler->MaxPropertyIndex, sHandler->InlineSlotCapacity);
Expand Down
12 changes: 11 additions & 1 deletion lib/Runtime/Library/JavascriptObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1538,7 +1538,17 @@ namespace Js
// else use enumerator to extract keys from source
else
{
AssignForGenericObjects(from, to, scriptContext);
DynamicObject* fromObj = JavascriptOperators::TryFromVar<DynamicObject>(from);
DynamicObject* toObj = JavascriptOperators::TryFromVar<DynamicObject>(to);
bool cloned = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

say we failed to clone for the first source, do we still attempt to do for the next sources?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check for cross-site object and just don't try that?


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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we exclude external objects (IsExternal()). Not sure what is the harm though.


In reply to: 174883937 [](ancestors = 174883937,174883683)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add case for cross-site object, I'm not sure about external, but I'll exclude them for good measure

if (toObj && fromObj && toObj->GetType() == scriptContext->GetLibrary()->GetObjectType())
{
cloned = toObj->TryCopy(fromObj);
}
if(!cloned)
{
AssignForGenericObjects(from, to, scriptContext);
}
}
}

Expand Down
97 changes: 92 additions & 5 deletions lib/Runtime/Types/DynamicObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ namespace Js
int propertyCount = typeHandler->GetPropertyCount();
int inlineSlotCapacity = GetTypeHandler()->GetInlineSlotCapacity();
int inlineSlotCount = min(inlineSlotCapacity, propertyCount);
Var * srcSlots = reinterpret_cast<Var*>(reinterpret_cast<size_t>(instance) + typeHandler->GetOffsetOfInlineSlots());
Field(Var) * dstSlots = reinterpret_cast<Field(Var)*>(reinterpret_cast<size_t>(this) + typeHandler->GetOffsetOfInlineSlots());
Field(Var)* srcSlots = instance->GetInlineSlots();
Field(Var)* dstSlots = this->GetInlineSlots();
#if !FLOATVAR
ScriptContext * scriptContext = this->GetScriptContext();
#endif
Expand Down Expand Up @@ -755,6 +755,94 @@ namespace Js
}
}

Field(Var)* DynamicObject::GetInlineSlots() const
{
return reinterpret_cast<Field(Var)*>(reinterpret_cast<size_t>(this) + this->GetOffsetOfInlineSlots());
}

bool DynamicObject::TryCopy(DynamicObject* from)
{
// Validate that objects are compatible
if (this->GetTypeHandler()->GetInlineSlotCapacity() != from->GetTypeHandler()->GetInlineSlotCapacity())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth extracting the compatibility checks to its own function? I'm wondering if it would be beneficial for the JIT to be able to generate a fast path for object.assign({}, obj) - in which case, as a simplification, it could call this function as a helper?

{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: inline slot capacity doesn't match, from: %u, to: %u\n"),
from->GetTypeHandler()->GetInlineSlotCapacity(),
this->GetTypeHandler()->GetInlineSlotCapacity());
}
return false;
}
if (!from->GetTypeHandler()->IsPathTypeHandler())
{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: Don't have PathTypeHandler\n"));
}
return false;
}
if (PathTypeHandlerBase::FromTypeHandler(from->GetTypeHandler())->HasAccessors())
{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: type handler has accessors\n"));
}
return false;
}
if (this->GetPrototype() != from->GetPrototype())
{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: Protoytypes don't match\n"));
}
return false;
}
if (!from->GetTypeHandler()->AllPropertiesAreEnumerable())
{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: from obj has non-enumerable properties\n"));
}
return false;
}

// Share the type
Copy link
Contributor

@digitalinfinity digitalinfinity Mar 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a comment here that ShareType has side effects so it should be done last #Resolved

if (!from->GetDynamicType()->ShareType())
{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: failed to share type\n"));
}
return false;
}
Copy link
Collaborator

@agarwal-sandeep agarwal-sandeep Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add test cases for these ifs and boundary cases for aux and inline slot capacities? #Resolved


// Update this object
this->ReplaceType(from->GetDynamicType());
this->InitSlots(this);
const int slotCapacity = this->GetTypeHandler()->GetSlotCapacity();
const uint16 inlineSlotCapacity = this->GetTypeHandler()->GetInlineSlotCapacity();
const int auxSlotCapacity = slotCapacity - inlineSlotCapacity;

if (auxSlotCapacity > 0)
{
CopyArray(this->auxSlots, auxSlotCapacity, from->auxSlots, auxSlotCapacity);
}
if (inlineSlotCapacity != 0)
{
Field(Var)* thisInlineSlots = this->GetInlineSlots();
Field(Var)* fromInlineSlots = from->GetInlineSlots();

CopyArray(thisInlineSlots, inlineSlotCapacity, fromInlineSlots, inlineSlotCapacity);
}

if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy succeeded\n"));
}

return true;
}

bool
DynamicObject::GetHasNoEnumerableProperties()
{
Expand Down Expand Up @@ -892,10 +980,9 @@ namespace Js
TTD::NSSnapObjects::StdExtractSetKindSpecificInfo<void*, TTD::NSSnapObjects::SnapObjectType::SnapDynamicObject>(objData, nullptr);
}

Js::Var const* DynamicObject::GetInlineSlots_TTD() const
Field(Js::Var) const* DynamicObject::GetInlineSlots_TTD() const
{
return reinterpret_cast<Var const*>(
reinterpret_cast<size_t>(this) + this->GetTypeHandler()->GetOffsetOfInlineSlots());
return this->GetInlineSlots();
}

Js::Var const* DynamicObject::GetAuxSlots_TTD() const
Expand Down
7 changes: 5 additions & 2 deletions lib/Runtime/Types/DynamicObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ namespace Js
void ReplaceType(DynamicType * type);
void ReplaceTypeWithPredecessorType(DynamicType * previousType);

Field(Var)* GetInlineSlots() const;

protected:
DEFINE_VTABLE_CTOR(DynamicObject, RecyclableObject);
DEFINE_MARSHAL_OBJECT_TO_SCRIPT_CONTEXT(DynamicObject);
Expand Down Expand Up @@ -144,7 +146,6 @@ namespace Js
Var GetSlot(int index);
Var GetInlineSlot(int index);
Var GetAuxSlot(int index);

#if DBG
void SetSlot(PropertyId propertyId, bool allowLetConst, int index, Var value);
void SetInlineSlot(PropertyId propertyId, bool allowLetConst, int index, Var value);
Expand Down Expand Up @@ -210,6 +211,8 @@ namespace Js
void InvalidateHasOnlyWritableDataPropertiesInPrototypeChainCacheIfPrototype();
void ResetObject(DynamicType* type, BOOL keepProperties);

bool TryCopy(DynamicObject* from);

virtual void SetIsPrototype();

bool HasLockedType() const;
Expand Down Expand Up @@ -344,7 +347,7 @@ namespace Js
virtual TTD::NSSnapObjects::SnapObjectType GetSnapTag_TTD() const override;
virtual void ExtractSnapObjectDataInto(TTD::NSSnapObjects::SnapObject* objData, TTD::SlabAllocator& alloc) override;

Js::Var const* GetInlineSlots_TTD() const;
Field(Js::Var) const* GetInlineSlots_TTD() const;
Js::Var const* GetAuxSlots_TTD() const;

#if ENABLE_OBJECT_SOURCE_TRACKING
Expand Down
8 changes: 8 additions & 0 deletions test/Object/assign.baseline
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
ObjectCopy succeeded
ObjectCopy: Can't copy: Don't have PathTypeHandler
ObjectCopy: Can't copy: type handler has accessors
ObjectCopy: Can't copy: type handler has accessors
ObjectCopy: Can't copy: Protoytypes don't match
ObjectCopy: Can't copy: from obj has non-enumerable properties
ObjectCopy succeeded
pass
108 changes: 108 additions & 0 deletions test/Object/assign.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
//-------------------------------------------------------------------------------------------------------
// Copyright (C) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
//-------------------------------------------------------------------------------------------------------

WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");

var tests = [
{
name: "simple copy",
body: function ()
{
let orig = {};
orig.a = 1;
orig.b = "asdf";
let newObj = Object.assign({}, orig);
assert.areEqual(newObj.b, orig.b);
assert.areEqual(newObj.a, orig.a);
}
},
{
name: "non-path type handler",
body: function ()
{
let orig = {};
orig.a = 1;
orig.b = "asdf";
delete orig.a;
let newObj = Object.assign({}, orig);
assert.areEqual(newObj.b, orig.b);
assert.areEqual(newObj.a, orig.a);
}
},
{
name: "has getter",
body: function ()
{
let orig = {};
orig.a = 1;
Object.defineProperty(orig, 'b', {
get: function() { return "asdf"; }, enumerable: true
});
Copy link
Collaborator

@agarwal-sandeep agarwal-sandeep Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimization should still kicking if prototype have getter? Can we add a test #Resolved

let newObj = Object.assign({}, orig);
assert.areEqual(newObj.b, orig.b);
assert.areEqual(newObj.a, orig.a);
}
},
{
name: "has setter",
body: function ()
{
let orig = {};
orig.a = 1;
Object.defineProperty(orig, 'b', {
set: function() { }, enumerable: true
});
let newObj = Object.assign({}, orig);
assert.areEqual(newObj.b, orig.b);
assert.areEqual(newObj.a, orig.a);
}
},
{
name: "different proto",
body: function ()
{
let protoObj = {};
let orig = Object.create(protoObj);
orig.a = 1;
orig.b = "asdf";

let newObj = Object.assign({}, orig);
assert.areEqual(newObj.b, orig.b);
assert.areEqual(newObj.a, orig.a);
}
},
{
name: "non-enumerable",
body: function ()
{
let orig = {};
orig.a = 1;
Object.defineProperty(orig, 'b', {
value: "asdf", enumerable: false
});

let newObj = Object.assign({}, orig);
assert.areEqual(newObj.b, undefined);
assert.areEqual(newObj.a, orig.a);
}
},
{
name: "proto accessor",
body: function ()
{
Object.defineProperty(Object.prototype, 'b', {
get: function() { return "asdf"; }
});
let orig = {};
orig.a = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious : symbol will not have impact, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, doesn't matter. I added symbol to the test case


let newObj = Object.assign({}, orig);
assert.areEqual(newObj.b, "asdf");
assert.areEqual(newObj.a, orig.a);
}
}
];

testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });
7 changes: 7 additions & 0 deletions test/Object/rlexe.xml
Original file line number Diff line number Diff line change
Expand Up @@ -436,4 +436,11 @@
<baseline />
</default>
</test>
<test>
<default>
<files>assign.js</files>
<compile-flags>-args summary -endargs -trace:ObjectCopy</compile-flags>
<baseline>assign.baseline</baseline>
</default>
</test>
</regress-exe>