Bug 197998 - Add extra information to dumpJITMemory
Summary: Add extra information to dumpJITMemory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-17 13:34 PDT by Tadeu Zagallo
Modified: 2019-05-18 00:13 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.48 KB, patch)
2019-05-17 13:39 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2019-05-17 14:28 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (7.38 KB, patch)
2019-05-17 23:32 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-05-17 13:34:22 PDT
...
Comment 1 Tadeu Zagallo 2019-05-17 13:39:42 PDT
Created attachment 370142 [details]
Patch
Comment 2 Tadeu Zagallo 2019-05-17 14:28:11 PDT
Created attachment 370151 [details]
Patch
Comment 3 Saam Barati 2019-05-17 23:08:44 PDT
Comment on attachment 370151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370151&action=review

r=me with one nit. Going to cq+ for now so this lands, feel free to just commit the followup

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:568
> +        ASSERT(dumpJITMemoryLock.isLocked());

Maybe also assert this in enqueue flush? Alternatively, take locker as parameter

> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:596
> +        flushQueue.get()->dispatchAfter(Seconds(Options::dumpJITMemoryFlushInterval()), [] {

nit: doesn’t Ref’s operator -> handle this? I don’t think you need the get()

> Source/JavaScriptCore/runtime/Options.h:521
> +    v(double, dumpJITMemoryFlushInterval, 10, Restricted, nullptr) \

Nit: I’d include the interval in the name or at least in the description of the option.
Comment 4 Tadeu Zagallo 2019-05-17 23:22:47 PDT
Comment on attachment 370151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370151&action=review

Thanks for the review!

>> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:568
>> +        ASSERT(dumpJITMemoryLock.isLocked());
> 
> Maybe also assert this in enqueue flush? Alternatively, take locker as parameter

Yeah, I was just being lazy about having to pass it to `write` too, but I think it's actually better. I'll just add it as a parameter.

>> Source/JavaScriptCore/jit/ExecutableAllocator.cpp:596
>> +        flushQueue.get()->dispatchAfter(Seconds(Options::dumpJITMemoryFlushInterval()), [] {
> 
> nit: doesn’t Ref’s operator -> handle this? I don’t think you need the get()

The `get()` is called in the never destroyed and the `->` in the Ref.

>> Source/JavaScriptCore/runtime/Options.h:521
>> +    v(double, dumpJITMemoryFlushInterval, 10, Restricted, nullptr) \
> 
> Nit: I’d include the interval in the name or at least in the description of the option.

Done, added it to the description.
Comment 5 Tadeu Zagallo 2019-05-17 23:32:24 PDT
Created attachment 370188 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2019-05-18 00:12:23 PDT
Comment on attachment 370188 [details]
Patch for landing

Clearing flags on attachment: 370188

Committed r245499: <https://trac.webkit.org/changeset/245499>
Comment 7 WebKit Commit Bot 2019-05-18 00:12:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-05-18 00:13:58 PDT
<rdar://problem/50915611>