Summary: | [bmalloc][Linux] Add support for memory status calculation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||||
Component: | bmalloc | Assignee: | Zan Dobersek <zan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aperez, cgarcia, ews-watchlist, ggaren, loic.yhuel, olivier.blin, pnormand, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Zan Dobersek
2019-03-19 01:34:20 PDT
Created attachment 365137 [details]
WIP
Created attachment 366486 [details]
Patch
Attachment 366486 [details] did not pass style-queue:
ERROR: Source/bmalloc/bmalloc/AvailableMemory.cpp:47: "algorithm" already included at Source/bmalloc/bmalloc/AvailableMemory.cpp:38 [build/include] [4]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
*** Bug 184684 has been marked as a duplicate of this bug. *** Comment on attachment 366486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366486&action=review > Source/bmalloc/bmalloc/AvailableMemory.cpp:112 > + if (s_singleton.statm.fd != -1) > + s_singleton.statm.valid = true; Why don't we just check fd instead of having both fd and valid? > Source/bmalloc/bmalloc/AvailableMemory.cpp:119 > + if (!statm.valid) Here we could just return early if fd == -1 > Source/bmalloc/bmalloc/AvailableMemory.cpp:123 > + ssize_t numBytes = pread(statm.fd, statmBuffer.data(), statmBuffer.size(), 0); Why using pread if the offset is always 0? > Source/bmalloc/bmalloc/AvailableMemory.cpp:144 > + } Is open/close so heavy operation? is it worth keeping the fd open forever? We are reading /proc so we are not even going to disk. > Source/bmalloc/bmalloc/AvailableMemory.cpp:146 > + long numPages { 0 }; We don't need to cache this, it's used only once in call_once() Comment on attachment 366486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366486&action=review >> Source/bmalloc/bmalloc/AvailableMemory.cpp:123 >> + ssize_t numBytes = pread(statm.fd, statmBuffer.data(), statmBuffer.size(), 0); > > Why using pread if the offset is always 0? Cause any other read operation advances the file position to the end of the file. So for every further read, that offset has to be brought back to the start of the file. >> Source/bmalloc/bmalloc/AvailableMemory.cpp:144 >> + } > > Is open/close so heavy operation? is it worth keeping the fd open forever? We are reading /proc so we are not even going to disk. They're separate operations, meaning that two additional syscalls would have to be done for every read. With the persistent fd we can instead only use one to both bring back the file position to the start and read the contents. Created attachment 367037 [details]
Patch
Attachment 367037 [details] did not pass style-queue:
ERROR: Source/bmalloc/bmalloc/AvailableMemory.cpp:47: "algorithm" already included at Source/bmalloc/bmalloc/AvailableMemory.cpp:38 [build/include] [4]
Total errors found: 1 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 367037 [details] Patch Clearing flags on attachment: 367037 Committed r244244: <https://trac.webkit.org/changeset/244244> All reviewed patches have been landed. Closing bug. Committed r244422: <https://trac.webkit.org/changeset/244422> Yusuke: your latest patch includes <array> under a BPLATFORM(IOS_FAMILY) guard, while this is necessary for BOS(LINUX) instead. (In reply to Olivier Blin from comment #13) > Yusuke: your latest patch includes <array> under a BPLATFORM(IOS_FAMILY) > guard, while this is necessary for BOS(LINUX) instead. No, this is not under BPLATFORM(IOS_FAMILY) guard. *** Bug 199515 has been marked as a duplicate of this bug. *** |