[webkit-reviews] review granted: [Bug 201994] Inline caching is wrong for custom accessors and custom values : [Attachment 379590] patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 201994: Inline caching is wrong for custom accessors and custom values
https://bugs.webkit.org/show_bug.cgi?id=201994

Attachment 379590: patch

https://bugs.webkit.org/attachment.cgi?id=379590&action=review




--- Comment #15 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 379590
  --> https://bugs.webkit.org/attachment.cgi?id=379590
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379590&action=review

r=me

> Source/JavaScriptCore/bytecode/AccessCase.cpp:-313
> -    return
m_conditionSet.structuresEnsureValidityAssumingImpurePropertyWatchpoint();

Don't we need to check `if (!m_conditionSet.isValid())`? If we can ensure that
this m_conditionSet is always valid, can we add assert instead?

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:-65
> -    return (numberOfConditionsWithKind(PropertyCondition::Presence) == 1) !=
(numberOfConditionsWithKind(PropertyCondition::Equivalence) == 1);

I think implementing this inline here seems better than iterating three times.
if (size() != 1)
    return false;
const ObjectPropertyCondition& condition = *this.begin();
switch (condition.kind()) {
case PropertyCondition::Presence:
case PropertyCondition::Equivalence:
case PropertyCondition::CustomFunctionEquivalence:
    return true;
...
    return false;
}
return false;

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:62
> +class AdaptiveValueStructureStubClearingWatchpoint final : public
AdaptiveInferredPropertyValueWatchpointBase {

Personally, I think we should rename
AdaptiveInferredPropertyValueWatchpointBase to something like
`AdaptiveInferredPropertyValueWatchpointCollectionBase` or something because it
is not a Watchpoint in fact (It is not inheriting Watchpoint).
Can you add it as a FIXME comment? Or can you rename it?

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:111
> +    Bag<WTF::Variant<StructureTransitionStructureStubClearingWatchpoint,
AdaptiveValueStructureStubClearingWatchpoint>> m_watchpoints;

IIRC, this watchpoint class is allocated so many. So, we should save memory as
much as possible.
Do we have any ideas? If we do not have an idea for now, please put a FIXME
about memory consumption.

> Source/JavaScriptCore/jit/Repatch.cpp:552
> +				   vm, codeBlock, exec, structure, slot.base(),
ident.impl(), 0);

I think `static_cast<unsigned>(PropertyAttribute::None)` is better instead of
`0`, it is ugly but anyway, it is more readable than 0.

> Source/JavaScriptCore/runtime/ObjectPropertyChangeAdaptiveWatchpoint.h:32
> +template<typename WatchpointSet>

Nice. At some point in the future, we should do this renaming for the other
places too :P