| Summary: | Array unshift/shift should not race against the AI in the compiler thread. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, fpizlo, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Mark Lam
2018-12-17 20:27:17 PST
Created attachment 357527 [details]
proposed patch.
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 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 on attachment 357527 [details] proposed patch. Clearing flags on attachment: 357527 Committed r239325: <https://trac.webkit.org/changeset/239325> All reviewed patches have been landed. Closing bug. |