Summary: | Add extra information to dumpJITMemory | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> | ||||||||
Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Tadeu Zagallo
2019-05-17 13:34:22 PDT
Created attachment 370142 [details]
Patch
Created attachment 370151 [details]
Patch
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 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. Created attachment 370188 [details]
Patch for landing
Comment on attachment 370188 [details] Patch for landing Clearing flags on attachment: 370188 Committed r245499: <https://trac.webkit.org/changeset/245499> All reviewed patches have been landed. Closing bug. |