optimize object.assign({}, obj) #4817
Conversation
lib/Runtime/Types/DynamicObject.cpp
Outdated
| } | ||
| return false; | ||
| } | ||
| if (!from->GetTypeHandler()->IsPathTypeHandler()) |
There was a problem hiding this comment.
IsPathTypeHandler [](start = 37, length = 17)
nitpicking: Should this and ShareType check be done first? Seems like these conditions are more frequent to fail? #Resolved
There was a problem hiding this comment.
i'll move PathTypeHandler check up, but I put ShareType last since it has side effects in case it succeeds #Closed
| Output::Print(_u("ObjectCopy: Can't copy: failed to share type\n")); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can we add test cases for these ifs and boundary cases for aux and inline slot capacities? #Resolved
lib/Runtime/Types/DynamicObject.cpp
Outdated
| } | ||
| return false; | ||
| } | ||
| if(PathTypeHandlerBase::FromTypeHandler(from->GetTypeHandler())->HasAccessors()) |
There was a problem hiding this comment.
HasAccessors [](start = 73, length = 12)
Does this cover both getter and setter? #Resolved
test/Object/rlexe.xml
Outdated
| <test> | ||
| <default> | ||
| <files>assign.js</files> | ||
| <compile-flags>-args summary -endargs</compile-flags> |
There was a problem hiding this comment.
Shouldn't we have -trace:ObjectCopy so that we know the optimization is kicking in? #Resolved
There was a problem hiding this comment.
I had that for my testing, but traces in tests can get noisy so didn't put it here. I can though since this probably shouldn't change much #Closed
| orig.a = 1; | ||
| Object.defineProperty(orig, 'b', { | ||
| get: function() { return "asdf"; }, enumerable: true | ||
| }); |
There was a problem hiding this comment.
Optimization should still kicking if prototype have getter? Can we add a test #Resolved
lib/Runtime/Types/DynamicObject.cpp
Outdated
| bool DynamicObject::TryCopy(DynamicObject* from) | ||
| { | ||
| // Validate that objects are compatible | ||
| if (this->GetTypeHandler()->GetInlineSlotCapacity() != from->GetTypeHandler()->GetInlineSlotCapacity()) |
There was a problem hiding this comment.
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?
lib/Runtime/Types/DynamicObject.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
| // Share the type |
There was a problem hiding this comment.
Might be worth adding a comment here that ShareType has side effects so it should be done last #Resolved
| get: function() { return "asdf"; } | ||
| }); | ||
| let orig = {}; | ||
| orig.a = 1; |
There was a problem hiding this comment.
Curious : symbol will not have impact, right?
There was a problem hiding this comment.
nope, doesn't matter. I added symbol to the test case
| AssignForGenericObjects(from, to, scriptContext); | ||
| DynamicObject* fromObj = JavascriptOperators::TryFromVar<DynamicObject>(from); | ||
| DynamicObject* toObj = JavascriptOperators::TryFromVar<DynamicObject>(to); | ||
| bool cloned = false; |
There was a problem hiding this comment.
say we failed to clone for the first source, do we still attempt to do for the next sources?
There was a problem hiding this comment.
Do we need to check for cross-site object and just don't try that?
In reply to: 174883683 [](ancestors = 174883683)
There was a problem hiding this comment.
Should we exclude external objects (IsExternal()). Not sure what is the harm though.
In reply to: 174883937 [](ancestors = 174883937,174883683)
There was a problem hiding this comment.
I'll add case for cross-site object, I'm not sure about external, but I'll exclude them for good measure
Merge pull request #4817 from MikeHolman:objassign In case of simple object.assign({}, obj) we can memcpy the properties over to the new object.
In case of simple object.assign({}, obj) we can memcpy the properties over to the new object.