Bug 230357 - Improve access case printing and show inline capacity for structures
Summary: Improve access case printing and show inline capacity for structures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-16 10:02 PDT by Justin Michaud
Modified: 2021-09-17 08:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (12.07 KB, patch)
2021-09-16 10:03 PDT, Justin Michaud
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
How it looks (88.37 KB, image/png)
2021-09-16 10:07 PDT, Justin Michaud
no flags Details
Patch (12.10 KB, patch)
2021-09-16 10:25 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (12.06 KB, patch)
2021-09-16 16:05 PDT, Justin Michaud
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (12.10 KB, patch)
2021-09-16 16:31 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (11.53 KB, patch)
2021-09-16 16:47 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (10.74 KB, patch)
2021-09-16 16:51 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2021-09-16 10:02:01 PDT
Improve access case printing and show inline capacity for structures
Comment 1 Justin Michaud 2021-09-16 10:03:41 PDT
Created attachment 438361 [details]
Patch
Comment 2 Justin Michaud 2021-09-16 10:06:13 PDT
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
Comment 3 Justin Michaud 2021-09-16 10:07:06 PDT
Created attachment 438363 [details]
How it looks

^ how it looks
Comment 4 Justin Michaud 2021-09-16 10:25:45 PDT
Created attachment 438365 [details]
Patch
Comment 5 Justin Michaud 2021-09-16 16:05:50 PDT
Created attachment 438411 [details]
Patch
Comment 6 Justin Michaud 2021-09-16 16:31:05 PDT
Created attachment 438416 [details]
Patch
Comment 7 Saam Barati 2021-09-16 16:39:13 PDT
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 8 Yusuke Suzuki 2021-09-16 16:39:41 PDT
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 9 Saam Barati 2021-09-16 16:40:34 PDT
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.
Comment 10 Justin Michaud 2021-09-16 16:42:56 PDT
> 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
Comment 11 Justin Michaud 2021-09-16 16:47:04 PDT
Created attachment 438419 [details]
Patch
Comment 12 Mark Lam 2021-09-16 16:48:05 PDT
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 { ?
Comment 13 Justin Michaud 2021-09-16 16:51:55 PDT
Created attachment 438420 [details]
Patch
Comment 14 Justin Michaud 2021-09-16 16:53:08 PDT
(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 :)
Comment 15 EWS 2021-09-17 08:58:17 PDT
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].
Comment 16 Radar WebKit Bug Importer 2021-09-17 08:59:16 PDT
<rdar://problem/83242945>