Summary: | [JSC] Add concurrency-aware version of isCallable / isConstructor to make it usable in DFG compiler | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | annulen, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2020-08-21 17:55:06 PDT
Created attachment 407035 [details]
Patch
Created attachment 407036 [details]
Patch
Created attachment 407037 [details]
Patch
Comment on attachment 407037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407037&action=review > Source/JavaScriptCore/runtime/Concurrency.h:34 > + OnMainThread, > + Concurrently, to make these names match, I'd do: OnMainThread, OnConcurrentThread MainThread, ConcurrentThread Main, Concurrent > Source/JavaScriptCore/runtime/JSCellInlines.h:254 > + else { Do we need an else here, since previous "if constexpr" ends in a return? > Source/JavaScriptCore/runtime/JSCellInlines.h:257 > + if (type() == InternalFunctionType) Maybe add a FIXME + link to bug about your plan to when NPAPI is removed b/c its concurrency issue, and rename getCallData to indicate it can be concurrent? Or perhaps, if that doesn't work out, add a new getCallDataConcurrently version? > Source/JavaScriptCore/runtime/JSCellInlines.h:272 > + else { Do we need an else here, since previous "if constexpr" ends in a return? Comment on attachment 407037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407037&action=review Thanks! >> Source/JavaScriptCore/runtime/Concurrency.h:34 >> + Concurrently, > > to make these names match, I'd do: > OnMainThread, OnConcurrentThread > > MainThread, ConcurrentThread > > Main, Concurrent Changing it to MainThread / ConcurrentThread. >> Source/JavaScriptCore/runtime/JSCellInlines.h:254 >> + else { > > Do we need an else here, since previous "if constexpr" ends in a return? Strictly speaking, this else makes else branch `if-constexpr-else`, but in this case, the above branch returns so maybe this does not affect on the generated code. Removed. >> Source/JavaScriptCore/runtime/JSCellInlines.h:257 >> + if (type() == InternalFunctionType) > > Maybe add a FIXME + link to bug about your plan to when NPAPI is removed b/c its concurrency issue, and rename getCallData to indicate it can be concurrent? > > Or perhaps, if that doesn't work out, add a new getCallDataConcurrently version? Added FIXME. >> Source/JavaScriptCore/runtime/JSCellInlines.h:272 >> + else { > > Do we need an else here, since previous "if constexpr" ends in a return? Removed because of the same reason above. Committed r266107: <https://trac.webkit.org/changeset/266107> |