Bug 195938 – [bmalloc][Linux] Add support for memory status calculation

• Component: bmalloc
• Status: RESOLVED FIXED

[Zan Dobersek]

[bmalloc][Linux] Add support for memory status calculation

[Zan Dobersek]

Created attachment 365137 [details]
WIP

[Zan Dobersek]

Created attachment 366486 [details]
Patch

[EWS Watchlist]

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.

[Zan Dobersek]

*** Bug 184684 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

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()

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 367037 [details]
Patch

[EWS Watchlist]

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.

[Zan Dobersek]

Comment on attachment 367037 [details]
Patch

Clearing flags on attachment: 367037

Committed r244244: <https://trac.webkit.org/changeset/244244>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49883046>

[Yusuke Suzuki]

Committed r244422: <https://trac.webkit.org/changeset/244422>

[Olivier Blin]

Yusuke: your latest patch includes <array> under a BPLATFORM(IOS_FAMILY) guard, while this is necessary for BOS(LINUX) instead.

[Yusuke Suzuki]

(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.

[Adrian Perez]

*** Bug 199515 has been marked as a duplicate of this bug. ***