Bug 192795 - Array unshift/shift should not race against the AI in the compiler thread.
Summary: Array unshift/shift should not race against the AI in the compiler thread.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-17 20:27 PST by Mark Lam
Modified: 2018-12-17 22:57 PST (History)
9 users (show)

See Also:


Attachments
proposed patch. (6.95 KB, patch)
2018-12-17 20:48 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2018-12-17 20:27:17 PST
<rdar://problem/46724263>
Comment 1 Mark Lam 2018-12-17 20:48:07 PST
Created attachment 357527 [details]
proposed patch.
Comment 2 Saam Barati 2018-12-17 21:30:46 PST
Comment on attachment 357527 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:16
> +        operations can move values around in the butterfly.  Hence, the fact that AI has

Do any operations do this without grabbing the lock? Or does this kind of moving around require holding the lock?
Comment 3 Mark Lam 2018-12-17 22:14:52 PST
Comment on attachment 357527 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/ChangeLog:16
>> +        operations can move values around in the butterfly.  Hence, the fact that AI has
> 
> Do any operations do this without grabbing the lock? Or does this kind of moving around require holding the lock?

I can't think of any other operations that keeps the moves the m_sparseMap around the butterfly like this.  I also did a survey of setButterfly(), nukeStructureAndSetButterfly(), the setting of arrayStorage()->m_indexBias, and the setting of arrayStorage()->m_sparseMap.  AFAICT, in all cases, we either
(1) allocating a new butterfly and fully initializing it before setting it in the object, which means just fetching the butterfly pointer is sufficient to guarantee we can read a meaningful value from it if the structure after is still the same, or
(2) the butterfly is already in ArrayStorage format, and we're setting the m_sparseMap either to a new sparse map or to null.  Other than unshift and shift, I didn't see anything that moves the location of m_sparseMap in the butterfly.  

So, I think we're good to go.
Comment 4 WebKit Commit Bot 2018-12-17 22:56:59 PST
Comment on attachment 357527 [details]
proposed patch.

Clearing flags on attachment: 357527

Committed r239325: <https://trac.webkit.org/changeset/239325>
Comment 5 WebKit Commit Bot 2018-12-17 22:57:00 PST
All reviewed patches have been landed.  Closing bug.