[tracing] Display the locked size of discardable memory segment.

[tracing] Display the locked size of discardable memory segment.

The locked size of the discardable memory segments is now displayed on
tracing. The browser side can only tell if the whole segment is locked
or not, if the segment is shared with a child. The child side can also
tell how much memory in the segment is locked.

BUG=529943
Committed: https://crrev.com/a29f3c7b772a058c69b91d393740b85d7d23526b
Cr-Commit-Position: refs/heads/master@{#352004}

[ssid, 2015-09-29 11:15:07]

ssid@chromium.org changed reviewers:
+ reveman@chromium.org

[ssid, 2015-09-29 11:15:08]

+reveman
I am not sure how the shared segment locking works in child. I am guessing if a
small part of the segment is locked then the whole segment is considered locked?
So, I am currently adding both the values I think are useful.

PTAL, Thanks.

[reveman, 2015-09-29 12:29:10]

https://codereview.chromium.org/1374213002/diff/1/base/memory/discardable_sha...
File base/memory/discardable_shared_memory.h (right):

https://codereview.chromium.org/1374213002/diff/1/base/memory/discardable_sha...
base/memory/discardable_shared_memory.h:62: size_t LockedSize() const;
I assume you're interested in being able to call this from any process and not
just the process locking the segment. LockedSize() doesn't make sense for that
as only the process locking the memory will know how much is locked. "bool
IsMemoryLocked() const" would work. If you add such a function, please keep it
next to "IsMemoryResident()".

I'm not sure we should expose the value of locked_page_count_ as that will be
confusing from a multi-process usage pov. I think it might be better to keep
track of this in each child process using the DiscardableSharedMemoryHeap spans.

https://codereview.chromium.org/1374213002/diff/1/content/common/discardable_...
File content/common/discardable_shared_memory_heap.cc (right):

https://codereview.chromium.org/1374213002/diff/1/content/common/discardable_...
content/common/discardable_shared_memory_heap.cc:401:
obj_dump->AddScalar("client_locked_size",
I think it will be enough to dump the exact locked size. What's actually locked
depends on the OS and the browser process should be responsible for dumping
that.

[ssid, 2015-09-29 16:03:55]

not really sure about this change, PTAL.

https://codereview.chromium.org/1374213002/diff/1/base/memory/discardable_sha...
File base/memory/discardable_shared_memory.h (right):

https://codereview.chromium.org/1374213002/diff/1/base/memory/discardable_sha...
base/memory/discardable_shared_memory.h:62: size_t LockedSize() const;
On 2015/09/29 12:29:10, reveman (OOO Sept 24-29) wrote:
> I assume you're interested in being able to call this from any process and not
> just the process locking the segment. LockedSize() doesn't make sense for that
> as only the process locking the memory will know how much is locked. "bool
> IsMemoryLocked() const" would work. If you add such a function, please keep it
> next to "IsMemoryResident()".
> 
Maybe I should just rename it? instead of calculating these outside or make it
private method and friend or something?

> I'm not sure we should expose the value of locked_page_count_ as that will be
> confusing from a multi-process usage pov. I think it might be better to keep
> track of this in each child process using the DiscardableSharedMemoryHeap
spans.

Hm, this looks like lot of change to me, I have adding the lock state in span
also moved lock and unlock feature to span itself. This is because everytime
someone wants to unlock the span they should remember to update the span locked
state. Curently manager was just using span->shared_memory()->Lock, now it will
use span::Lock. If not for the lock mechanism, I would have removed the
ChildDiscardableSharedMemoryManager::Lock and directly called span->Lock in
DiscardableMemoryImpl. But this feels a bit weird that the impl now has two
choices, it can span->lock or manager->lock(span). But calling manager is thread
safe.
Also I had to hard code true and false for span constructor.

Should I just store the locked state and update it everytime memory is locked
and unlocked?

https://codereview.chromium.org/1374213002/diff/1/content/common/discardable_...
File content/common/discardable_shared_memory_heap.cc (right):

https://codereview.chromium.org/1374213002/diff/1/content/common/discardable_...
content/common/discardable_shared_memory_heap.cc:401:
obj_dump->AddScalar("client_locked_size",
On 2015/09/29 12:29:10, reveman (OOO Sept 24-29) wrote:
> I think it will be enough to dump the exact locked size. What's actually
locked
> depends on the OS and the browser process should be responsible for dumping
> that.

Sounds good.

https://codereview.chromium.org/1374213002/diff/20001/content/common/discarda...
File content/common/discardable_shared_memory_heap.h (right):

https://codereview.chromium.org/1374213002/diff/20001/content/common/discarda...
content/common/discardable_shared_memory_heap.h:31: bool shared_memory() {
return shared_memory_; }
I have also removed the getter here since it is no longer required, just added
isResident method

[ssid, 2015-09-29 16:05:00]

On 2015/09/29 16:03:55, ssid wrote:
> not really sure about this change, PTAL.
> 
>
https://codereview.chromium.org/1374213002/diff/1/base/memory/discardable_sha...
> File base/memory/discardable_shared_memory.h (right):
> 
>
https://codereview.chromium.org/1374213002/diff/1/base/memory/discardable_sha...
> base/memory/discardable_shared_memory.h:62: size_t LockedSize() const;
> On 2015/09/29 12:29:10, reveman (OOO Sept 24-29) wrote:
> > I assume you're interested in being able to call this from any process and
not
> > just the process locking the segment. LockedSize() doesn't make sense for
that
> > as only the process locking the memory will know how much is locked. "bool
> > IsMemoryLocked() const" would work. If you add such a function, please keep
it
> > next to "IsMemoryResident()".
> > 
> Maybe I should just rename it? instead of calculating these outside or make it
> private method and friend or something?
> 
> > I'm not sure we should expose the value of locked_page_count_ as that will
be
> > confusing from a multi-process usage pov. I think it might be better to keep
> > track of this in each child process using the DiscardableSharedMemoryHeap
> spans.
> 
> Hm, this looks like lot of change to me, I have adding the lock state in span
> also moved lock and unlock feature to span itself. This is because everytime
> someone wants to unlock the span they should remember to update the span
locked
> state. Curently manager was just using span->shared_memory()->Lock, now it
will
> use span::Lock. If not for the lock mechanism, I would have removed the
> ChildDiscardableSharedMemoryManager::Lock and directly called span->Lock in
> DiscardableMemoryImpl. But this feels a bit weird that the impl now has two
> choices, it can span->lock or manager->lock(span). But calling manager is
thread
> safe.
> Also I had to hard code true and false for span constructor.
> 
> Should I just store the locked state and update it everytime memory is locked
> and unlocked?
> 
>
https://codereview.chromium.org/1374213002/diff/1/content/common/discardable_...
> File content/common/discardable_shared_memory_heap.cc (right):
> 
>
https://codereview.chromium.org/1374213002/diff/1/content/common/discardable_...
> content/common/discardable_shared_memory_heap.cc:401:
> obj_dump->AddScalar("client_locked_size",
> On 2015/09/29 12:29:10, reveman (OOO Sept 24-29) wrote:
> > I think it will be enough to dump the exact locked size. What's actually
> locked
> > depends on the OS and the browser process should be responsible for dumping
> > that.
> 
> Sounds good.
> 
>
https://codereview.chromium.org/1374213002/diff/20001/content/common/discarda...
> File content/common/discardable_shared_memory_heap.h (right):
> 
>
https://codereview.chromium.org/1374213002/diff/20001/content/common/discarda...
> content/common/discardable_shared_memory_heap.h:31: bool shared_memory() {
> return shared_memory_; }
> I have also removed the getter here since it is no longer required, just added
> isResident method

Also it is not an urgent review, it can wait till you get back from vacation :)

[reveman, 2015-09-29 22:58:31]

https://codereview.chromium.org/1374213002/diff/20001/base/memory/discardable...
File base/memory/discardable_shared_memory.cc (right):

https://codereview.chromium.org/1374213002/diff/20001/base/memory/discardable...
base/memory/discardable_shared_memory.cc:344: if (!shared_memory_.memory())
nit: Can this be a DCHECK as IsMemoryResident?

https://codereview.chromium.org/1374213002/diff/20001/content/common/discarda...
File content/common/discardable_shared_memory_heap.h (right):

https://codereview.chromium.org/1374213002/diff/20001/content/common/discarda...
content/common/discardable_shared_memory_heap.h:36: void Unlock(size_t
page_size);
I'd like to keep Span class as a simple container without any real logic. Can we
instead add getter/setters for is_locked? and use them from the manager?

[ssid, 2015-09-30 19:24:31]

Patchset #3 (id:40001) has been deleted

[ssid, 2015-09-30 19:27:21]

Removed code from Span class. Updating the is_locked whenever necessary. I don't
think I should change the constructor of Span since, the methods that create
span sometimes do not know about the locked state. Child manager updates when
necessary.

PTAL.

https://codereview.chromium.org/1374213002/diff/20001/base/memory/discardable...
File base/memory/discardable_shared_memory.cc (right):

https://codereview.chromium.org/1374213002/diff/20001/base/memory/discardable...
base/memory/discardable_shared_memory.cc:344: if (!shared_memory_.memory())
On 2015/09/29 22:58:31, reveman (OOO Sept 24-29) wrote:
> nit: Can this be a DCHECK as IsMemoryResident?

Done.

[reveman, 2015-10-01 04:08:54]

lgtm

[ssid, 2015-10-01 10:22:29]

ssid@chromium.org changed reviewers:
+ avi@chromium.org, thestig@chromium.org

[ssid, 2015-10-01 10:22:29]

This CL adds support for getting the locked size.

+avi for discardable manager changes in content.

+thestig for base/memory changes.

PTAL, thanks

[reveman, 2015-10-01 10:34:40]

sorry, forgot to include some nits in my previous review. lgtm after fixing
these.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
File content/common/discardable_shared_memory_heap.cc (right):

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:371: size_t
locked_size_in_bytes = 0;
nit: locked_objects_size_in_bytes to be consistent with above variables.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:378: const size_t span_size =
span->length_ * block_size_;
How about computing allocated_objects_size_in_blocks and
locked_objects_size_in_blocks and convert to bytes when you call AddScalar
instead multiplying with block_size_ for each iteration? that also removes the
need for this temporary variable.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:404: locked_size_in_bytes);
why no static_cast<uint64_t> here but we have one above? either way is fine with
me as long as it's consistent.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
File content/common/discardable_shared_memory_heap.h (right):

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.h:45: bool is_locked_;
please initialize this to false in ctor.

https://codereview.chromium.org/1374213002/diff/60001/content/common/host_dis...
File content/common/host_discardable_shared_memory_manager.cc (right):

https://codereview.chromium.org/1374213002/diff/60001/content/common/host_dis...
content/common/host_discardable_shared_memory_manager.cc:184: const size_t
mapped_size = segment->memory()->mapped_size();
fyi: mapped_size() being lower case means it's a cheap getter so you can avoid
this temporary variable if you like

[ssid, 2015-10-01 14:08:47]

Thanks, made changes.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
File content/common/discardable_shared_memory_heap.cc (right):

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:371: size_t
locked_size_in_bytes = 0;
On 2015/10/01 10:34:39, reveman (OOO Sept 24-29) wrote:
> nit: locked_objects_size_in_bytes to be consistent with above variables.

Done.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:378: const size_t span_size =
span->length_ * block_size_;
On 2015/10/01 10:34:39, reveman (OOO Sept 24-29) wrote:
> How about computing allocated_objects_size_in_blocks and
> locked_objects_size_in_blocks and convert to bytes when you call AddScalar
> instead multiplying with block_size_ for each iteration? that also removes the
> need for this temporary variable.

Done.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:404: locked_size_in_bytes);
On 2015/10/01 10:34:39, reveman (OOO Sept 24-29) wrote:
> why no static_cast<uint64_t> here but we have one above? either way is fine
with
> me as long as it's consistent.

I think initially it was added since the AddScalar did not accept 64 bit
numbers, that changed now, cleaning up.

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
File content/common/discardable_shared_memory_heap.h (right):

https://codereview.chromium.org/1374213002/diff/60001/content/common/discarda...
content/common/discardable_shared_memory_heap.h:45: bool is_locked_;
On 2015/10/01 10:34:40, reveman (OOO Sept 24-29) wrote:
> please initialize this to false in ctor.

Done.

https://codereview.chromium.org/1374213002/diff/60001/content/common/host_dis...
File content/common/host_discardable_shared_memory_manager.cc (right):

https://codereview.chromium.org/1374213002/diff/60001/content/common/host_dis...
content/common/host_discardable_shared_memory_manager.cc:184: const size_t
mapped_size = segment->memory()->mapped_size();
On 2015/10/01 10:34:40, reveman (OOO Sept 24-29) wrote:
> fyi: mapped_size() being lower case means it's a cheap getter so you can avoid
> this temporary variable if you like

Added only because of the line sizes. don't really have an opinion

[reveman, 2015-10-01 14:39:00]

https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
File base/memory/discardable_shared_memory.cc (right):

https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
base/memory/discardable_shared_memory.cc:344: DCHECK(IsMemoryResident());
hm, I was just thinking we'd DCHECK(shared_memory_.memory()) here. Why
DCHECK(IsMemoryResident())?

https://codereview.chromium.org/1374213002/diff/80001/content/common/discarda...
File content/common/discardable_shared_memory_heap.cc (right):

https://codereview.chromium.org/1374213002/diff/80001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:381:
locked_objects_size_in_bytes += span->is_locked_ ? span->length_ : 0;
nit: this is a bit confusing. you're adding a number of blocks to variables
called "in_bytes". please change the name of these variables to "in_blocks" and
use a different set of variables for "in_bytes"

[ssid, 2015-10-01 14:45:46]

Patchset #5 (id:100001) has been deleted

[ssid, 2015-10-01 14:48:24]

https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
File base/memory/discardable_shared_memory.cc (right):

https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
base/memory/discardable_shared_memory.cc:344: DCHECK(IsMemoryResident());
On 2015/10/01 14:39:00, reveman (OOO Sept 24-29) wrote:
> hm, I was just thinking we'd DCHECK(shared_memory_.memory()) here. Why
> DCHECK(IsMemoryResident())?

You told me to make it DCHECK(IsMemoryResident) in the previous comment. I think
it will be only called by host and for resident memory.

https://codereview.chromium.org/1374213002/diff/80001/content/common/discarda...
File content/common/discardable_shared_memory_heap.cc (right):

https://codereview.chromium.org/1374213002/diff/80001/content/common/discarda...
content/common/discardable_shared_memory_heap.cc:381:
locked_objects_size_in_bytes += span->is_locked_ ? span->length_ : 0;
On 2015/10/01 14:39:00, reveman (OOO Sept 24-29) wrote:
> nit: this is a bit confusing. you're adding a number of blocks to variables
> called "in_bytes". please change the name of these variables to "in_blocks"
and
> use a different set of variables for "in_bytes"

Yeah, I thought you did not like introducing new variables, so tried to limit..
changed.

[reveman, 2015-10-01 14:54:14]

https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
File base/memory/discardable_shared_memory.cc (right):

https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
base/memory/discardable_shared_memory.cc:344: DCHECK(IsMemoryResident());
On 2015/10/01 at 14:48:24, ssid wrote:
> On 2015/10/01 14:39:00, reveman (OOO Sept 24-29) wrote:
> > hm, I was just thinking we'd DCHECK(shared_memory_.memory()) here. Why
> > DCHECK(IsMemoryResident())?
> 
> You told me to make it DCHECK(IsMemoryResident) in the previous comment. I
think it will be only called by host and for resident memory.

By "DCHECK as IsMemoryResident" I meant DCHECK in the same way
IsMemoryResident() does. Sorry for the confusion. I think
DCHECK(shared_memory_.memory()) is better here.

[ssid, 2015-10-01 14:56:22]

On 2015/10/01 14:54:14, reveman (OOO Sept 24-29) wrote:
>
https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
> File base/memory/discardable_shared_memory.cc (right):
> 
>
https://codereview.chromium.org/1374213002/diff/80001/base/memory/discardable...
> base/memory/discardable_shared_memory.cc:344: DCHECK(IsMemoryResident());
> On 2015/10/01 at 14:48:24, ssid wrote:
> > On 2015/10/01 14:39:00, reveman (OOO Sept 24-29) wrote:
> > > hm, I was just thinking we'd DCHECK(shared_memory_.memory()) here. Why
> > > DCHECK(IsMemoryResident())?
> > 
> > You told me to make it DCHECK(IsMemoryResident) in the previous comment. I
> think it will be only called by host and for resident memory.
> 
> By "DCHECK as IsMemoryResident" I meant DCHECK in the same way
> IsMemoryResident() does. Sorry for the confusion. I think
> DCHECK(shared_memory_.memory()) is better here.

Oh, sorry. Fixed.

[reveman, 2015-10-01 15:03:35]

lgtm

[Avi (use Gerrit), 2015-10-01 15:11:28]

lgtm

stampity stamp

[Lei Zhang, 2015-10-01 18:39:32]

Can you exercise DiscardableSharedMemory::IsMemoryLocked() in base_unittests?

[ssid, 2015-10-01 19:09:29]

Patchset #7 (id:160001) has been deleted

[ssid, 2015-10-01 19:10:40]

Added test coverage, PTAL. Thanks

[reveman, 2015-10-01 19:14:12]

https://codereview.chromium.org/1374213002/diff/180001/base/memory/discardabl...
File base/memory/discardable_shared_memory_unittest.cc (right):

https://codereview.chromium.org/1374213002/diff/180001/base/memory/discardabl...
base/memory/discardable_shared_memory_unittest.cc:36:
ASSERT_TRUE(memory.IsMemoryLocked());
why not EXPECT_* here an below? ASSERT_* is typically for when going on would be
meaningless and likely cause a crash.

[ssid, 2015-10-01 19:30:21]

Thanks, made them expect*.

https://codereview.chromium.org/1374213002/diff/180001/base/memory/discardabl...
File base/memory/discardable_shared_memory_unittest.cc (right):

https://codereview.chromium.org/1374213002/diff/180001/base/memory/discardabl...
base/memory/discardable_shared_memory_unittest.cc:36:
ASSERT_TRUE(memory.IsMemoryLocked());
On 2015/10/01 19:14:12, reveman (OOO Sept 24-29) wrote:
> why not EXPECT_* here an below? ASSERT_* is typically for when going on would
be
> meaningless and likely cause a crash.

Done.

[Lei Zhang, 2015-10-01 19:56:11]

lgtm

[ssid, 2015-10-02 11:29:05]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-10-02 11:29:05]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1374213002/#ps200001
(title: "Change to expect.")

[commit-bot: I haz the power, 2015-10-02 11:29:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1374213002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1374213002/200001

[commit-bot: I haz the power, 2015-10-02 11:36:26]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2015-10-02 11:37:32]

Patchset 8 (id:??) landed as
https://crrev.com/a29f3c7b772a058c69b91d393740b85d7d23526b
Cr-Commit-Position: refs/heads/master@{#352004}

[ssid, 2015-10-09 10:34:47]

A revert of this CL (patchset #8 id:200001) has been created in
https://codereview.chromium.org/1386333003/ by ssid@chromium.org.

The reason for reverting is: Speculative revert for the bug:
www.crbug.com/541029.