Bug 197998 – Add extra information to dumpJITMemory

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 370142 [details]
Patch

[Tadeu Zagallo]

Created attachment 370151 [details]
Patch

[Saam Barati]

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.

[Tadeu Zagallo]

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.

[Tadeu Zagallo]

Created attachment 370188 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370188 [details]
Patch for landing

Clearing flags on attachment: 370188

Committed r245499: <https://trac.webkit.org/changeset/245499>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50915611>