Bug 195938 - [bmalloc][Linux] Add support for memory status calculation
Summary: [bmalloc][Linux] Add support for memory status calculation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
: 184684 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-03-19 01:34 PDT by Zan Dobersek
Modified: 2019-04-19 11:47 PDT (History)
9 users (show)

See Also:


Attachments
WIP (6.12 KB, patch)
2019-03-19 01:34 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (7.33 KB, patch)
2019-04-02 04:35 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2019-04-09 04:32 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2019-03-19 01:34:20 PDT
[bmalloc][Linux] Add support for memory status calculation
Comment 1 Zan Dobersek 2019-03-19 01:34:52 PDT
Created attachment 365137 [details]
WIP
Comment 2 Zan Dobersek 2019-04-02 04:35:00 PDT
Created attachment 366486 [details]
Patch
Comment 3 Build Bot 2019-04-02 04:36:47 PDT
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.
Comment 4 Zan Dobersek 2019-04-05 05:20:23 PDT
*** Bug 184684 has been marked as a duplicate of this bug. ***
Comment 5 Carlos Garcia Campos 2019-04-08 01:07:58 PDT
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 6 Zan Dobersek 2019-04-08 01:30:59 PDT
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.
Comment 7 Zan Dobersek 2019-04-09 04:32:07 PDT
Created attachment 367037 [details]
Patch
Comment 8 Build Bot 2019-04-09 04:33:55 PDT
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 9 Zan Dobersek 2019-04-13 23:46:20 PDT
Comment on attachment 367037 [details]
Patch

Clearing flags on attachment: 367037

Committed r244244: <https://trac.webkit.org/changeset/244244>
Comment 10 Zan Dobersek 2019-04-13 23:46:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-04-13 23:49:22 PDT
<rdar://problem/49883046>
Comment 12 Yusuke Suzuki 2019-04-18 00:30:22 PDT
Committed r244422: <https://trac.webkit.org/changeset/244422>
Comment 13 Olivier Blin 2019-04-19 08:22:00 PDT
Yusuke: your latest patch includes <array> under a BPLATFORM(IOS_FAMILY) guard, while this is necessary for BOS(LINUX) instead.
Comment 14 Yusuke Suzuki 2019-04-19 11:47:27 PDT
(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.