Bug 186954 – We need to have a getDirectConcurrently for use in the compilers

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

[Keith Miller]

ObjectPropertyCondition and Graph::tryGetConstantProperty should lock the structure before calling getDirect

[Keith Miller]

Created attachment 343405 [details]
Patch

[Saam Barati]

Comment on attachment 343405 [details]
Patch

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

> Source/JavaScriptCore/bytecode/PropertyCondition.cpp:400
> +        // If we are flattening the structure at this time we might shrink the butterfly and read garbage.
> +        ConcurrentJSLocker locker(structure->lock());
> +        if (!structure->isValidOffset(offset()))
> +            return PropertyCondition();
> +        value = base->getDirect(offset());

Don't we know if we're doing this concurrently or not? Maybe just lock in that scenario?

[Keith Miller]

Comment on attachment 343405 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/PropertyCondition.cpp:400
>> +        value = base->getDirect(offset());
> 
> Don't we know if we're doing this concurrently or not? Maybe just lock in that scenario?

Fair point, I'll make that change.

[Keith Miller]

Comment on attachment 343405 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/PropertyCondition.cpp:400
>>> +        value = base->getDirect(offset());
>> 
>> Don't we know if we're doing this concurrently or not? Maybe just lock in that scenario?
> 
> Fair point, I'll make that change.

Actually, we do a lot of things using the concurrent versions of methods inside PropertyCondition when we are on the mutator. We should fix all of them in a more consistent way. I'm just gonna file a bug.

[Keith Miller]

Created attachment 343410 [details]
Patch

[Keith Miller]

Created attachment 343411 [details]
Patch

[Keith Miller]

rdar://problem/41385437

[Mark Lam]

Comment on attachment 343411 [details]
Patch

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

r=me.  If you haven't already done so, I think you should also remove the "if (!value)" check I added in PropertyCondition::isValidValueForAttributes() due to https://bugs.webkit.org/show_bug.cgi?id=186943.  Your changes in attemptToMakeEquivalenceWithoutBarrier() makes that unnecessary.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1268
> +    // object pointer such that the inlinecaches told us that the object had a structure that it

missing space between "inline" and "caches".

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1270
> +    // caches had alraedy seen. And then the processor reordered the stores. Seems unlikely and

/alraedy/already/.

[Keith Miller]

Comment on attachment 343411 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1268
>> +    // object pointer such that the inlinecaches told us that the object had a structure that it
> 
> missing space between "inline" and "caches".

Fixed

>> Source/JavaScriptCore/dfg/DFGGraph.cpp:1270
>> +    // caches had alraedy seen. And then the processor reordered the stores. Seems unlikely and
> 
> /alraedy/already/.

Fixed

[Keith Miller]

(In reply to Mark Lam from comment #8)
> r=me.  If you haven't already done so, I think you should also remove the
> "if (!value)" check I added in
> PropertyCondition::isValidValueForAttributes() due to
> https://bugs.webkit.org/show_bug.cgi?id=186943.  Your changes in
> attemptToMakeEquivalenceWithoutBarrier() makes that unnecessary.

I think I'm just going to undo my change to attemptToMakeEquivalenceWithoutBarrier() instead. That check makes more sense in isValidValueForAttributes().

[Keith Miller]

Committed r233124: <https://trac.webkit.org/changeset/233124>