Implement committed physical memory stats for Linux. (issue 11066118)

6 views
Skip to first unread message

al...@chromium.org

unread,
Oct 11, 2012, 9:17:23 AM10/11/12
to mstar...@chromium.org, da...@chromium.org, veg...@chromium.org, v8-...@googlegroups.com, yu...@chromium.org, loi...@chromium.org
Reviewers: Michael Starzinger, danno, Vyacheslav Egorov,

Message:
Michael,

I've implemented the committed physical size approximation without use of
mincore which happened to be prohibited in the sandbox. PTAL.

Description:
Implement committed physical memory stats for Linux.

The patch introduces CommittedPhysicalMemory function to
the Heap class that reports committed *physical* memory acquired
for the heap from the OS.
It is important because some OSes may defer actual committment on e.g.
first access to the region.
So reporting just plain committed size led to various weird artifacts
like showing V8 allocated memory higher than the whole process
private size.

BUG=v8:2191


Please review this at https://chromiumcodereview.appspot.com/11066118/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M include/v8.h
M src/api.cc
M src/heap.h
M src/heap.cc
M src/platform-cygwin.cc
M src/platform-freebsd.cc
M src/platform-linux.cc
M src/platform-macos.cc
M src/platform-nullos.cc
M src/platform-openbsd.cc
M src/platform-solaris.cc
M src/platform-win32.cc
M src/platform.h
M src/spaces-inl.h
M src/spaces.h
M src/spaces.cc


mstar...@chromium.org

unread,
Oct 16, 2012, 9:01:06 AM10/16/12
to al...@chromium.org, da...@chromium.org, veg...@chromium.org, v8-...@googlegroups.com, yu...@chromium.org, loi...@chromium.org

https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc
File src/spaces.cc (right):

https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc#newcode882
src/spaces.cc:882:
MemoryChunk::UpdateHighWaterMark(allocation_info_.top);
This function is called by PagedSpace::SlowAllocateRaw which uses the
free-list allocator. So the updating should be covered by
PagedSpace::SetTop(). Can we drop this?

https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc#newcode1142
src/spaces.cc:1142:
MemoryChunk::UpdateHighWaterMark(allocation_info_.top);
I think this one is better bottle-necked in
NewSpace::UpdateAllocationInfo() which is called after the two
semi-spaces have been flipped. Can we move it there?

https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc#newcode1227
src/spaces.cc:1227:
MemoryChunk::UpdateHighWaterMark(allocation_info_.top);
This one will also be covered by NewSpace::UpdateAllocationInfo(). Can
we drop this?

https://chromiumcodereview.appspot.com/11066118/

al...@chromium.org

unread,
Oct 16, 2012, 9:44:41 AM10/16/12
to mstar...@chromium.org, da...@chromium.org, veg...@chromium.org, v8-...@googlegroups.com, yu...@chromium.org, loi...@chromium.org
Thanks for the review. PTAL.

P.S. Looks like I rebaselined the patch at some point so diff with the
previous
patch is screwed. I'm sorry.


https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc
File src/spaces.cc (right):

https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc#newcode882
src/spaces.cc:882:
MemoryChunk::UpdateHighWaterMark(allocation_info_.top);
On 2012/10/16 13:01:06, Michael Starzinger wrote:
> This function is called by PagedSpace::SlowAllocateRaw which uses the
free-list
> allocator. So the updating should be covered by PagedSpace::SetTop().
Can we
> drop this?

Done.

https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc#newcode1142
src/spaces.cc:1142:
MemoryChunk::UpdateHighWaterMark(allocation_info_.top);
On 2012/10/16 13:01:06, Michael Starzinger wrote:
> I think this one is better bottle-necked in
NewSpace::UpdateAllocationInfo()
> which is called after the two semi-spaces have been flipped. Can we
move it
> there?

Done.

https://chromiumcodereview.appspot.com/11066118/diff/1/src/spaces.cc#newcode1227
src/spaces.cc:1227:
MemoryChunk::UpdateHighWaterMark(allocation_info_.top);
On 2012/10/16 13:01:06, Michael Starzinger wrote:
> This one will also be covered by NewSpace::UpdateAllocationInfo(). Can
we drop
> this?

Done.

https://chromiumcodereview.appspot.com/11066118/
Reply all
Reply to author
Forward
0 new messages