...
Created attachment 459379 [details] Patch
Comment on attachment 459379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459379&action=review > Source/JavaScriptCore/ChangeLog:3 > + REGRESSION (r89377554): [iOS] %2 RAMification This is not a WebKit revision.
(In reply to Alexey Proskuryakov from comment #2) > Comment on attachment 459379 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=459379&action=review > > > Source/JavaScriptCore/ChangeLog:3 > > + REGRESSION (r89377554): [iOS] %2 RAMification > > This is not a WebKit revision. Sorry, I though "r" stands for radar.
Comment on attachment 459379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459379&action=review Looking good, I left a few comments that might help you with your issues. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14866 > + slowCases.append(m_jit.branchCompactPtr(CCallHelpers::NotEqual, scratch1GPR, CCallHelpers::Address(structureGPR, Structure::classInfoOffset()), scratch2GPR)); Can we make branchCompactPtr do the right thing so we don't need these #ifs everywhere? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14872 > > emitAllocateJSObjectWithKnownSize<JSInternalPromise>(resultGPR, structureGPR, butterfly, scratch1GPR, scratch2GPR, slowCases, sizeof(JSInternalPromise)); looks like scratch2 is still live? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14924 > slowCases.append(m_jit.branchPtr(CCallHelpers::NotEqual, scratch1GPR, CCallHelpers::Address(structureGPR, Structure::globalObjectOffset()))); ditto for both > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:16838 > + return m_out.lShr(structure, m_out.constInt32(StructureID::encodeShiftAmount)); need a b3 cast? This would only cause a validation error when JSC is running the B3 compiler, not a runtime error > Source/JavaScriptCore/heap/TinyBloomFilter.h:36 > + using StorageSize = WTF::Compacted<nullptr_t>::StorageSize; We can get rid of Bits, and get rid of the #if here. StorageSize should pick the right size, right? Maybe let's name it StorageType or StorageBits or Bits? > Source/JavaScriptCore/jit/AssemblyHelpers.h:-1625 > - load64(MacroAssembler::Address(structure, Structure::structureIDOffset()), scratch); Yusuke should leave a comment here. This seems correct, but I am not sure if we can leave a JSCell in this state. We might need to do this atomically to avoid GC tearing. This might happen if the GC would visit this cell in between these two stores. > Source/JavaScriptCore/jit/AssemblyHelpers.h:1930 > + { Let's define this for all platforms and have it do the right thing > Source/JavaScriptCore/runtime/Structure.h:955 > + static constexpr int s_maxTransitionLength = 64; // macOS|iOS: 4 bytes Not sue about the comment here? Also, constexpr values don't necessarily have to take up any space. Static values also don't contribute to object size. > Source/WTF/wtf/CompactPtr.h:28 > +#include <cstdint> Not sure we need all of these > Source/WTF/wtf/CompactRefPtr.h:38 > + WTF_MAKE_FAST_ALLOCATED; I don't think we want this at all. We want this to be used only as a member of a class. Also, maybe we should see if we can assert that it isn't stack allocated, since that will break leaks.
(In reply to Yijia Huang from comment #3) > (In reply to Alexey Proskuryakov from comment #2) > > Comment on attachment 459379 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=459379&action=review > > > > > Source/JavaScriptCore/ChangeLog:3 > > > + REGRESSION (r89377554): [iOS] %2 RAMification > > > > This is not a WebKit revision. > > Sorry, I though "r" stands for radar. For radars, we use rdar://000000
cc'ing the radar bot and adding the InRadar tag: <rdar://89377554>
Comment on attachment 459379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459379&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14866 >> + slowCases.append(m_jit.branchCompactPtr(CCallHelpers::NotEqual, scratch1GPR, CCallHelpers::Address(structureGPR, Structure::classInfoOffset()), scratch2GPR)); > > Can we make branchCompactPtr do the right thing so we don't need these #ifs everywhere? m_jit.branchCompactPtr requires additional scratchGPR. How can we handle: /Volumes/WebKit/ios/OpenSource/Source/JavaScriptCore/jit/AssemblyHelpers.h:1928:88: error: unused parameter 'scratch' [-Werror,-Wunused-parameter] Jump branchCompactPtr(RelationalCondition cond, GPRReg left, Address right, GPRReg scratch) , when it is not iOS platform? >> Source/JavaScriptCore/heap/TinyBloomFilter.h:36 >> + using StorageSize = WTF::Compacted<nullptr_t>::StorageSize; > > We can get rid of Bits, and get rid of the #if here. StorageSize should pick the right size, right? Maybe let's name it StorageType or StorageBits or Bits? The Bits is actually being used somewhere else. Should we use `uintptr_t` for `Compacted::StorageSize` for the non-iOS platforms instead of T*? Then, we can directly use `using StorageSize = WTF::Compacted<nullptr_t>::StorageSize;`
Created attachment 459467 [details] Patch
Created attachment 459469 [details] Patch
Created attachment 459470 [details] Patch
Comment on attachment 459470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459470&action=review > Source/JavaScriptCore/jit/AssemblyHelpers.h:1936 > + (void)scratch; Use UNUSED_PARAM(scratch).
Pull request: https://github.com/WebKit/WebKit/pull/658
Comment on attachment 459470 [details] Patch This patch is obsolete. The current patch is at the PR on GitHub.
Pull request: https://github.com/WebKit/WebKit/pull/889
Pull request: https://github.com/WebKit/WebKit/pull/894
Pull request: https://github.com/WebKit/WebKit/pull/896
Pull request: https://github.com/WebKit/WebKit/pull/963
Pull request: https://github.com/WebKit/WebKit/pull/966
Created attachment 459705 [details] Patch
Pull request: https://github.com/WebKit/WebKit/pull/983
Committed r295008 (251103@main): <https://commits.webkit.org/251103@main> Reviewed commits have been landed. Closing PR #658 and removing active labels.
Landed Commit: https://github.com/WebKit/WebKit/commit/771382d8a3be082672a5f071b45375d6d3d36219 Fix for iOS debug build: https://github.com/WebKit/WebKit/commit/b9d14b3aaf16c9647fe0bb6abac487c5585ce737