Worth looking into it. https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=object-assign-es5
We should have some fixup to remove runtime branches. For example, PutByVal(CellUse, StringUse, UntypedUse).
Created attachment 320257 [details] Patch
Comment on attachment 320257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320257&action=review > Source/JavaScriptCore/dfg/DFGOperations.cpp:151 > + RELEASE_ASSERT(base->isObject()); what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object?
Comment on attachment 320257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320257&action=review >> Source/JavaScriptCore/dfg/DFGOperations.cpp:151 >> + RELEASE_ASSERT(base->isObject()); > > what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object? This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects.
Comment on attachment 320257 [details] Patch Cool! I think that we can make the Generic case even better by implementing it as an inline cache. Then we can have baseline, DFG, and FTL share that inline cache. And we could use it for better array access profiling.
(In reply to Filip Pizlo from comment #5) > Comment on attachment 320257 [details] > Patch > > Cool! > > I think that we can make the Generic case even better by implementing it as > an inline cache. Then we can have baseline, DFG, and FTL share that inline > cache. And we could use it for better array access profiling. It sounds nice. We already have IC for GetByVal if it sees only one String/Symbol Identifier. But adding this type of IC (String/Symbol specialization) could further improve performance in baseline, DFG, and FTL.
Committed r221793: <http://trac.webkit.org/changeset/221793>
Comment on attachment 320257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320257&action=review >>> Source/JavaScriptCore/dfg/DFGOperations.cpp:151 >>> + RELEASE_ASSERT(base->isObject()); >> >> what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object? > > This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects. Right, but your patch looks like it’s calling this from non-direct case. Am I missing something?
Comment on attachment 320257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320257&action=review >>>> Source/JavaScriptCore/dfg/DFGOperations.cpp:151 >>>> + RELEASE_ASSERT(base->isObject()); >>> >>> what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object? >> >> This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects. > > Right, but your patch looks like it’s calling this from non-direct case. Am I missing something? This "direct" template param is true only when it's used for PutByValDirect.
Comment on attachment 320257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320257&action=review >>>>> Source/JavaScriptCore/dfg/DFGOperations.cpp:151 >>>>> + RELEASE_ASSERT(base->isObject()); >>>> >>>> what guarantees this? Inside Fixup, you speculate Cell, not object? If it's guaranteed, why don't you speculate object? >>> >>> This is originally guaranteed in operationPutByVal generic functions too. This is because we emit `op_put_by_val` with direct flag only for objects. >> >> Right, but your patch looks like it’s calling this from non-direct case. Am I missing something? > > This "direct" template param is true only when it's used for PutByValDirect. Sorry I was completely misreading the code. This LGTM
<rdar://problem/34693626>