Improve access case printing and show inline capacity for structures
Created attachment 438361 [details] Patch
Comment on attachment 438361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438361&action=review > Source/JavaScriptCore/heap/Heap.cpp:1497 > if (UNLIKELY(m_verifier)) { This is better because the gc verifier crashes before you see the heap verifier output
Created attachment 438363 [details] How it looks ^ how it looks
Created attachment 438365 [details] Patch
Created attachment 438411 [details] Patch
Created attachment 438416 [details] Patch
Comment on attachment 438416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438416&action=review > Source/JavaScriptCore/runtime/Structure.cpp:1327 > + "/", (uint32_t)(reinterpret_cast<uintptr_t>(sid)), ", ", I feel like "(uint32_t)(reinterpret_cast<uintptr_t>(sid))" can just be "id()" :-) I think that reinterpret_cast is just there to print as hex? Which is kinda silly. We should eventually just make a hex printer that doesn't require a pointer. Also, do we really need to print the structure id int both as hex and as base 10?
Comment on attachment 438416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438416&action=review > Source/JavaScriptCore/runtime/JSCJSValue.cpp:309 > + out.print("Symbol: ", RawPointer(asCell()), " (", jsCast<Symbol*>(asCell())->description(), ")"); Symbol::description returns String held by Symbol. So, this function is not safe to be called concurrently. Since dump can be called concurrently (e.g. from DFG compiler thread), we cannot use it. >> Source/JavaScriptCore/runtime/Structure.cpp:1327 >> + auto* sid = reinterpret_cast<void*>(id()); >> + out.print(RawPointer(this), ":[", RawPointer(sid), >> + "/", (uint32_t)(reinterpret_cast<uintptr_t>(sid)), ", ", > > I feel like "(uint32_t)(reinterpret_cast<uintptr_t>(sid))" can just be "id()" :-) > > I think that reinterpret_cast is just there to print as hex? Which is kinda silly. We should eventually just make a hex printer that doesn't require a pointer. > > Also, do we really need to print the structure id int both as hex and as base 10? What is the difference between sid and this reinterpret_casted sid?
Comment on attachment 438416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438416&action=review >>> Source/JavaScriptCore/runtime/Structure.cpp:1327 >>> + "/", (uint32_t)(reinterpret_cast<uintptr_t>(sid)), ", ", >> >> I feel like "(uint32_t)(reinterpret_cast<uintptr_t>(sid))" can just be "id()" :-) >> >> I think that reinterpret_cast is just there to print as hex? Which is kinda silly. We should eventually just make a hex printer that doesn't require a pointer. >> >> Also, do we really need to print the structure id int both as hex and as base 10? > > What is the difference between sid and this reinterpret_casted sid? actually, I don't really mind printing it both as hex/base 10. I'll leave it up to you what to do. My preference might actually be base 10 only.
> I feel like "(uint32_t)(reinterpret_cast<uintptr_t>(sid))" can just be > "id()" :-) I thought that too, but it fails on 32 bit because it is actually a Structure* there. > > I think that reinterpret_cast is just there to print as hex? Which is kinda > silly. We should eventually just make a hex printer that doesn't require a > pointer. yeah > Also, do we really need to print the structure id int both as hex and as > base 10? We don't *need* to, but I found it helpful because we sometimes print our literals in the assembly code as decimal. We use all three, (hex id, pointer, decimal), it sucks. > What is the difference between sid and this reinterpret_casted sid? base 10 / 16
Created attachment 438419 [details] Patch
Comment on attachment 438416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438416&action=review > Source/JavaScriptCore/heap/Heap.cpp:1501 > - if (UNLIKELY(Options::verifyGC())) > - verifyGC(); > - > if (UNLIKELY(m_verifier)) { > m_verifier->gatherLiveCells(HeapVerifier::Phase::AfterMarking); > m_verifier->verify(HeapVerifier::Phase::AfterMarking); > } > > + if (UNLIKELY(Options::verifyGC())) > + verifyGC(); > + You should do this change in a separate patch. It's unrelated, and seems random that it should be included here. The ChangeLog also makes no mention of it. > Source/JavaScriptCore/runtime/Structure.cpp:1325 > + auto* sid = reinterpret_cast<void*>(id()); Let's call this structureID to make it clear what it is. > Source/JavaScriptCore/runtime/Structure.cpp:1329 > + outOfLineSize(), "/", outOfLineCapacity(), "){"); nit: space between ) and { ?
Created attachment 438420 [details] Patch
(In reply to Mark Lam from comment #12) > Comment on attachment 438416 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=438416&action=review > > > Source/JavaScriptCore/heap/Heap.cpp:1501 > > - if (UNLIKELY(Options::verifyGC())) > > - verifyGC(); > > - > > if (UNLIKELY(m_verifier)) { > > m_verifier->gatherLiveCells(HeapVerifier::Phase::AfterMarking); > > m_verifier->verify(HeapVerifier::Phase::AfterMarking); > > } > > > > + if (UNLIKELY(Options::verifyGC())) > > + verifyGC(); > > + > > You should do this change in a separate patch. It's unrelated, and seems > random that it should be included here. The ChangeLog also makes no mention > of it. > > > Source/JavaScriptCore/runtime/Structure.cpp:1325 > > + auto* sid = reinterpret_cast<void*>(id()); > > Let's call this structureID to make it clear what it is. > > > Source/JavaScriptCore/runtime/Structure.cpp:1329 > > + outOfLineSize(), "/", outOfLineCapacity(), "){"); > > nit: space between ) and { ? done / done / intentional :)
Committed r282664 (241805@main): <https://commits.webkit.org/241805@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438420 [details].
<rdar://problem/83242945>