Bug 185695

Created attachment 340714 [details] it begins

Created attachment 340724 [details] a bit more

<rdar://problem/40373500>

Created attachment 340731 [details] maye it is written

Created attachment 340740 [details] it did things

Created attachment 340748 [details] it's a nice speed-up

Created attachment 340751 [details] the patch

Attachment 340751 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 5 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 340755 [details] the patch

Attachment 340755 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 5 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 340756 [details] the patch

Attachment 340756 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 5 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 340759 [details] the patch

Attachment 340759 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:37: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:38: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:39: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/bytecode/InstanceOfVariant.cpp:40: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 5 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 340759 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=340759&action=review r=me with nits. > Source/JavaScriptCore/bytecode/InstanceOfVariant.h:37 > + InstanceOfVariant() { } Use `InstanceOfVariant() = default;` > Source/JavaScriptCore/bytecode/InstanceOfVariant.h:61 > + JSObject* m_prototype = nullptr; Make this `JSObject* m_prototype { nullptr }`. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3389 > + if (base.m_structure.isInfinite() || baseSet.size() < base.m_structure.size()) Is `baseSet.size() < base.m_structure.size()` condition correct? If node->matchStructureData().variants have so many structures, which includes all the base.m_structure, baseSet.size() == base.m_structure.size(), but we have a chance to reduce # of variants. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3390 > + m_state.setFoundConstants(true); I think we have a chance to clear NodeMustGenerate in constant folding phase if the given abstract value's structure set is finite and all the structures are covered by MatchStructure's set. > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:520 > + RegisteredStructureSet structureSet; > + for (MatchStructureVariant& variant : data.variants) > + structureSet.add(variant.structure); > + addBaseCheck(indexInBlock, node, baseValue, structureSet); > + m_graph.convertToConstant( > + node, m_graph.freeze(jsBoolean(result == BooleanLattice::True))); `changed = true`? > Source/WTF/wtf/BooleanLattice.h:42 > +inline BooleanLattice lubBooleanLattice(BooleanLattice a, BooleanLattice b) > +{ > + return static_cast<BooleanLattice>(static_cast<uint8_t>(a) | static_cast<uintptr_t>(b)); > +} I think `leastUpperBoundOfBooleanLattices` is better since we already have bunch of `leastUpperBoundXXX` functions.

(In reply to Yusuke Suzuki from comment #15) > Comment on attachment 340759 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340759&action=review > > r=me with nits. > > > Source/JavaScriptCore/bytecode/InstanceOfVariant.h:37 > > + InstanceOfVariant() { } > > Use `InstanceOfVariant() = default;` Fixed. Can you tell me what the advantage of this is? > > > Source/JavaScriptCore/bytecode/InstanceOfVariant.h:61 > > + JSObject* m_prototype = nullptr; > > Make this `JSObject* m_prototype { nullptr }`. Fixed. > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3389 > > + if (base.m_structure.isInfinite() || baseSet.size() < base.m_structure.size()) > > Is `baseSet.size() < base.m_structure.size()` condition correct? If > node->matchStructureData().variants have so many structures, which includes > all the base.m_structure, baseSet.size() == base.m_structure.size(), but we > have a chance to reduce # of variants. You're right. This should be baseSet.size() < node->matchStructureData().variants.size(). I fixed it and will retest perf. > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:3390 > > + m_state.setFoundConstants(true); > > I think we have a chance to clear NodeMustGenerate in constant folding phase > if the given abstract value's structure set is finite and all the structures > are covered by MatchStructure's set. We don't usually clear NodeMustGenerate in other cases where this happens, so I didn't do it here. In FTL mode, it really doesn't matter, since we rely on B3 to do the real DCE. > > > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:520 > > + RegisteredStructureSet structureSet; > > + for (MatchStructureVariant& variant : data.variants) > > + structureSet.add(variant.structure); > > + addBaseCheck(indexInBlock, node, baseValue, structureSet); > > + m_graph.convertToConstant( > > + node, m_graph.freeze(jsBoolean(result == BooleanLattice::True))); > > `changed = true`? You're right! Fixed. > > > Source/WTF/wtf/BooleanLattice.h:42 > > +inline BooleanLattice lubBooleanLattice(BooleanLattice a, BooleanLattice b) > > +{ > > + return static_cast<BooleanLattice>(static_cast<uint8_t>(a) | static_cast<uintptr_t>(b)); > > +} > > I think `leastUpperBoundOfBooleanLattices` is better since we already have > bunch of `leastUpperBoundXXX` functions. Right. Fixed.

Comment on attachment 340759 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=340759&action=review >>> Source/JavaScriptCore/bytecode/InstanceOfVariant.h:37 >>> + InstanceOfVariant() { } >> >> Use `InstanceOfVariant() = default;` > > Fixed. Can you tell me what the advantage of this is? The compiler can offer a good information without adding an explicit annotation. For example, if all the members are constexpr-initialized, constructor becomes `constexpr` constructor :)

Landed in https://trac.webkit.org/changeset/232000/webkit