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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95cb7c5
optimize object.assign({}, obj)
MikeHolman Feb 27, 2018
2d4084b
fix xplat build; address comments
MikeHolman Mar 14, 2018
93212c8
add tests
MikeHolman Mar 14, 2018
1bb71ab
include trace, add another test case
MikeHolman Mar 15, 2018
9273771
address review comments
MikeHolman Mar 15, 2018
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
View file
Open in desktop
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
12 changes: 11 additions & 1 deletion lib/Runtime/Library/JavascriptObject.cpp
View file
Open in desktop
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
95 changes: 91 additions & 4 deletions lib/Runtime/Types/DynamicObject.cpp
View file
Open in desktop
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 (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;
}
if (!from->GetTypeHandler()->IsPathTypeHandler())
Copy link
Collaborator

@agarwal-sandeep agarwal-sandeep Mar 14, 2018 •
edited by MikeHolman
Loading

Choose a reason for hiding this comment

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

IsPathTypeHandler [](start = 37, length = 17)

nitpicking: Should this and ShareType check be done first? Seems like these conditions are more frequent to fail? #Resolved

Copy link
Contributor Author

@MikeHolman MikeHolman Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

i'll move PathTypeHandler check up, but I put ShareType last since it has side effects in case it succeeds #Closed

{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: Don't have PathTypeHandler\n"));
}
return false;
}
if(PathTypeHandlerBase::FromTypeHandler(from->GetTypeHandler())->HasAccessors())
Copy link
Collaborator

@agarwal-sandeep agarwal-sandeep Mar 14, 2018 •
edited by MikeHolman
Loading

Choose a reason for hiding this comment

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

HasAccessors [](start = 73, length = 12)

Does this cover both getter and setter? #Resolved

{
if (PHASE_TRACE1(ObjectCopyPhase))
{
Output::Print(_u("ObjectCopy: Can't copy: type handler has accessors\n"));
}
return false;
}

// Share the type
Copy link
Contributor

@digitalinfinity digitalinfinity Mar 15, 2018 •
edited by MikeHolman
Loading

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 •
edited by MikeHolman
Loading

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 = GetTypeHandler()->GetSlotCapacity();
const uint16 inlineSlotCapacity = 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 @@ -894,8 +982,7 @@ namespace Js

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
5 changes: 4 additions & 1 deletion lib/Runtime/Types/DynamicObject.h
View file
Open in desktop
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