Bug 158291 – canOptimizeStringObjectAccess should use ObjectPropertyConditions rather than structure watchpoints

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

canOptimizeStringObjectAccess should use ObjectPropertyConditions rather than structure watchpoints

[Keith Miller]

Created attachment 280294 [details]
Patch

[Keith Miller]

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.

[Keith Miller]

Created attachment 280296 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 280296 [details]
Patch for landing

Clearing flags on attachment: 280296

Committed r201584: <http://trac.webkit.org/changeset/201584>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

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()