<rdar://problem/54832121>
Created attachment 377620 [details] proposed patch.
Comment on attachment 377620 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=377620&action=review > Source/JavaScriptCore/runtime/JSArrayBufferView.h:177 > + enum ResultQuality { AccurateResult, RacyResult }; > + template<ResultQuality = AccurateResult> unsigned byteOffset(); This is OK for internal implementation. But I think usually we are using the name `xxxConcurrently()` for this type of function. So, I think `byteOffsetConcurrently()` would be better. > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:86 > ArrayBuffer* buffer = possiblySharedBuffer(); Is it safe to be called from DFG thread? If we go to JSArrayBufferView::slowDownAndWasteMemory, we allocate Butterfly in that function, and it is not allowed in concurrent compilers.
Comment on attachment 377620 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=377620&action=review >> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:86 >> ArrayBuffer* buffer = possiblySharedBuffer(); > > Is it safe to be called from DFG thread? If we go to JSArrayBufferView::slowDownAndWasteMemory, we allocate Butterfly in that function, and it is not allowed in concurrent compilers. Thanks. I will investigate.
Comment on attachment 377620 [details] proposed patch. Take out of review while this is being investigated.
Comment on attachment 377620 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=377620&action=review >>> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:86 >>> ArrayBuffer* buffer = possiblySharedBuffer(); >> >> Is it safe to be called from DFG thread? If we go to JSArrayBufferView::slowDownAndWasteMemory, we allocate Butterfly in that function, and it is not allowed in concurrent compilers. > > Thanks. I will investigate. Turns out, this is safe because the hasArrayBuffer() check above ensures that mode() is never FastTypedArray or OversizeTypedArray, and hence, we'll never call JSArrayBufferView::slowDownAndWasteMemory().
Created attachment 377929 [details] proposed patch.
Created attachment 377931 [details] proposed patch.
Comment on attachment 377931 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=377931&action=review > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91 > + ASSERT(isMainThread()); Looks like I can't assert this. Worker threads will not be happy.
Created attachment 377945 [details] proposed patch.
Comment on attachment 377931 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=377931&action=review >> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91 >> + ASSERT(isMainThread()); > > Looks like I can't assert this. Worker threads will not be happy. You can use `ASSERT(!isCompilationThread());` instead.
(In reply to Yusuke Suzuki from comment #10) > Comment on attachment 377931 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377931&action=review > > >> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91 > >> + ASSERT(isMainThread()); > > > > Looks like I can't assert this. Worker threads will not be happy. > > You can use `ASSERT(!isCompilationThread());` instead. Sounds good. Fixing.
Created attachment 377947 [details] proposed patch.
Comment on attachment 377947 [details] proposed patch. r=me
(In reply to Mark Lam from comment #8) > Comment on attachment 377931 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377931&action=review > > > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91 > > + ASSERT(isMainThread()); > > Looks like I can't assert this. Worker threads will not be happy. I think we made this mistake in another patch recently too, right?
(In reply to Saam Barati from comment #14) > (In reply to Mark Lam from comment #8) > > Comment on attachment 377931 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=377931&action=review > > > > > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91 > > > + ASSERT(isMainThread()); > > > > Looks like I can't assert this. Worker threads will not be happy. > > I think we made this mistake in another patch recently too, right? https://bugs.webkit.org/show_bug.cgi?id=201281
(In reply to Saam Barati from comment #15) > (In reply to Saam Barati from comment #14) > > (In reply to Mark Lam from comment #8) > > > Comment on attachment 377931 [details] > > > proposed patch. > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=377931&action=review > > > > > > > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91 > > > > + ASSERT(isMainThread()); > > > > > > Looks like I can't assert this. Worker threads will not be happy. > > > > I think we made this mistake in another patch recently too, right? > > https://bugs.webkit.org/show_bug.cgi?id=201281 In 201281, the mistake reduces the strength of the assertion for worker threads instead of overly asserting it (which resulted in crashes) like this case. Regardless, I'll fix it.
Thanks for the review. Landed in r249458: <http://trac.webkit.org/r249458>.