Bug 230357 – Improve access case printing and show inline capacity for structures

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Michaud]

Improve access case printing and show inline capacity for structures

[Justin Michaud]

Created attachment 438361 [details]
Patch

[Justin Michaud]

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

[Justin Michaud]

Created attachment 438363 [details]
How it looks

^ how it looks

[Justin Michaud]

Created attachment 438365 [details]
Patch

[Justin Michaud]

Created attachment 438411 [details]
Patch

[Justin Michaud]

Created attachment 438416 [details]
Patch

[Saam Barati]

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?

[Yusuke Suzuki]

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?

[Saam Barati]

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.

[Justin Michaud]

> 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

[Justin Michaud]

Created attachment 438419 [details]
Patch

[Mark Lam]

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 { ?

[Justin Michaud]

Created attachment 438420 [details]
Patch

[Justin Michaud]

(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 :)

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/83242945>