canOptimizeStringObjectAccess should use ObjectPropertyConditions rather than structure watchpoints
Created attachment 280294 [details] Patch
Comment on attachment 280294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280294&action=review > Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:65 > + return (numberOfConditionsWithKind(PropertyCondition::Presence) ^ numberOfConditionsWithKind(PropertyCondition::Equivalence)) == 1; Whoops, this should be (numberOfConditionsWithKind(PropertyCondition::Presence) == 1) != (numberOfConditionsWithKind(PropertyCondition::Equivalence) == 1). I'll fix before landing.
Created attachment 280296 [details] Patch for landing
Comment on attachment 280296 [details] Patch for landing Clearing flags on attachment: 280296 Committed r201584: <http://trac.webkit.org/changeset/201584>
All reviewed patches have been landed. Closing bug.
Comment on attachment 280296 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=280296&action=review Can you do a follow-up fix to clean up the API? > Source/JavaScriptCore/bytecode/ObjectPropertyCondition.cpp:52 > - return m_condition.isStillValidAssumingImpurePropertyWatchpoint(structure); > + return m_condition.isStillValidAssumingImpurePropertyWatchpoint(structure, m_object); This part is wrong. The documentation for structureEnsuresValidityAssumingImpurePropertyWatchpoint() says: // Checks if the object's structure claims that the property won't be intercepted. Validity // does not require watchpoints on the object. Passing a non-null object to PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint() means that it will return true in cases where we actually need to set watchpoints on the object. It may be that nobody calls isStillValidAssumingImpurePropertyWatchpoint() this way, but that's a super crazy assumption since the DFG assumes this meaning for structureEnsuresValidity(), which is supposed to only differ from structureEnsuresValidityAssumingImpurePropertyWatchpoint() in how it handles the impure property watchpoint thing. So, this patch maybe causes a correctness bug somewhere (maybe not), but definitely makes the API inconsistent. I don't think we want that. Can you add a new method instead? I guess it should be called ObjectPropertyCondition::isStillValidAssumingImpurePropertyWatchpoint()