Summary: | We should have a DFG intrinsic that checks if a value is a TypedArrayView | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, mark.lam, msaboff, saam | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Keith Miller
2016-06-22 16:58:14 PDT
Created attachment 281887 [details]
Patch
Created attachment 281888 [details]
Patch
Comment on attachment 281888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281888&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1276 > + if (!(child.m_type & ~SpecTypedArrayView)) { Shouldn't this also check !!child.m_type? > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1281 > + if (!(child.m_type & SpecTypedArrayView)) { ditto > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6121 > + m_out.branch(isCell(value, provenType(m_node->child1())), unsure(isCellCase), unsure(continuation)); I would guess the isCellCase is likely. > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:707 > + GlobalPropertyInfo(vm.propertyNames->builtinNames().isTypedArrayViewPrivateName(), privateFuncIsTypedArrayView, DontEnum | DontDelete | ReadOnly), Where is this used from besides the test? > Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:70 > + return JSValue::encode(jsBoolean(value.isObject() && isTypedView(value.getObject()->classInfo()->typedArrayStorageType))); I think this would be faster as: jsBoolean(value.isCell() && isTypedView(value.asCell()->classInfo()->typedArrayStorageType)) because most cells you will see are going to be objects. > Source/JavaScriptCore/tests/stress/istypedarrayview-intrinsic.js:4 > + let foo = createBuiltin(`(function (a) { "use strict"; return @isTypedArrayView(a); })`); Neat. Comment on attachment 281888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281888&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1281 >> + if (!(child.m_type & SpecTypedArrayView)) { > > ditto ignore this ditto Comment on attachment 281888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281888&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1276 >> + if (!(child.m_type & ~SpecTypedArrayView)) { > > Shouldn't this also check !!child.m_type? I'm pretty sure that we force OSR if the child has no speculated type. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6121 >> + m_out.branch(isCell(value, provenType(m_node->child1())), unsure(isCellCase), unsure(continuation)); > > I would guess the isCellCase is likely. While I agree with you in general, my thought process is that this node will be eliminated in most of the cases where the value is a TypedArray. In the cases where we can't eliminate the node I think its harder to claim the value is a usually a cell. >> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:707 >> + GlobalPropertyInfo(vm.propertyNames->builtinNames().isTypedArrayViewPrivateName(), privateFuncIsTypedArrayView, DontEnum | DontDelete | ReadOnly), > > Where is this used from besides the test? I'm going to use this when I move subarray to a builtin. Currently it is not used elsewhere. >> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:70 >> + return JSValue::encode(jsBoolean(value.isObject() && isTypedView(value.getObject()->classInfo()->typedArrayStorageType))); > > I think this would be faster as: > jsBoolean(value.isCell() && isTypedView(value.asCell()->classInfo()->typedArrayStorageType)) > because most cells you will see are going to be objects. Fixed. Created attachment 281892 [details]
Patch for landing
Comment on attachment 281892 [details] Patch for landing Clearing flags on attachment: 281892 Committed r202363: <http://trac.webkit.org/changeset/202363> All reviewed patches have been landed. Closing bug. |