[memory-infra] Make client discardable segments non-weak

[memory-infra] Make client discardable segments non-weak

The discardable segments shared between the processes do not add the
same global dump ids since the process id of the client is not available
in the manager. Temporarily we do not create weak dumps since the dump
provider is broken.

BUG=661257, 687399
Review-Url: https://codereview.chromium.org/2668193002
Cr-Commit-Position: refs/heads/master@{#448554}
Committed: https://chromium.googlesource.com/chromium/src/+/efed310cc305f0d89f9d1aa49fb5e0d4ec9ff2cd

[ssid, 2017-02-01 03:11:04]

The CQ bit was checked by ssid@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 03:11:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ssid, 2017-02-01 03:13:16]

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

[ssid, 2017-02-01 03:13:16]

+primiano this is temp fix to not show 0 here. Since it's weak the dumps
disappear now. I can't find anyway to get pid in the manager. We will soon
remove this code when shared_memory dumps have edges. So, meanwhile what do you
think about this?

[commit-bot: I haz the power, 2017-02-01 03:22:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 03:22:31]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Primiano Tucci (use gerrit), 2017-02-03 20:09:51]

On 2017/02/01 03:13:16, ssid wrote:
> +primiano this is temp fix to not show 0 here. Since it's weak the dumps
> disappear now. I can't find anyway to get pid in the manager. We will soon
> remove this code when shared_memory dumps have edges. So, meanwhile what do
you
> think about this?

So, I read both bugs linked and couldn't figure out still why at some point this
broke? do you have a quick summary? it was working before with weak dumps, so...
what changed?

[ssid, 2017-02-03 21:56:32]

On 2017/02/03 20:09:51, Primiano Tucci wrote:
> On 2017/02/01 03:13:16, ssid wrote:
> > +primiano this is temp fix to not show 0 here. Since it's weak the dumps
> > disappear now. I can't find anyway to get pid in the manager. We will soon
> > remove this code when shared_memory dumps have edges. So, meanwhile what do
> you
> > think about this?
> 
> So, I read both bugs linked and couldn't figure out still why at some point
this
> broke? do you have a quick summary? it was working before with weak dumps,
so...
> what changed?

I have explained in
https://bugs.chromium.org/p/chromium/issues/detail?id=661257#c4
The dump provider is broken because the global dump ids do not match. I cannot
figure out how to fix this because right now the discardable manager does not
know the process id for the clients. It only knows a client id. It is much
effort to figure out how to get pid in the manager now, given that we are going
to have shared_memory provider which will remove the use of this global dump is
since it will create the global dump ids. meanwhile when the shared_memory
provider is not ready yet, i am just changing the weak dumps to strong ones
which will cause only a slight discrepancy in the memory reporting (mostly 0 in
telemetry since we will never hit the case of browser clearing segments after
reaching 500MiB limit).

[ssid, 2017-02-03 21:59:11]

On 2017/02/03 21:56:32, ssid wrote:
> On 2017/02/03 20:09:51, Primiano Tucci wrote:
> > On 2017/02/01 03:13:16, ssid wrote:
> > > +primiano this is temp fix to not show 0 here. Since it's weak the dumps
> > > disappear now. I can't find anyway to get pid in the manager. We will soon
> > > remove this code when shared_memory dumps have edges. So, meanwhile what
do
> > you
> > > think about this?
> > 
> > So, I read both bugs linked and couldn't figure out still why at some point
> this
> > broke? do you have a quick summary? it was working before with weak dumps,
> so...
> > what changed?
> 
> I have explained in
> https://bugs.chromium.org/p/chromium/issues/detail?id=661257#c4
> The dump provider is broken because the global dump ids do not match. I cannot
> figure out how to fix this because right now the discardable manager does not
> know the process id for the clients. It only knows a client id. It is much
> effort to figure out how to get pid in the manager now, given that we are
going
> to have shared_memory provider which will remove the use of this global dump
is
> since it will create the global dump ids. meanwhile when the shared_memory
> provider is not ready yet, i am just changing the weak dumps to strong ones
> which will cause only a slight discrepancy in the memory reporting (mostly 0
in
> telemetry since we will never hit the case of browser clearing segments after
> reaching 500MiB limit).

A bit more clear: Currently we show 0 in renderer discardable because the global
dump ids do not match and all the child discardable dumps get ignored. This cl
just fixes this ignoring part.

[Primiano Tucci (use gerrit), 2017-02-06 10:48:16]

On 2017/02/03 21:59:11, ssid wrote:
> On 2017/02/03 21:56:32, ssid wrote:
> > On 2017/02/03 20:09:51, Primiano Tucci wrote:
> > > On 2017/02/01 03:13:16, ssid wrote:
> > > > +primiano this is temp fix to not show 0 here. Since it's weak the dumps
> > > > disappear now. I can't find anyway to get pid in the manager. We will
soon
> > > > remove this code when shared_memory dumps have edges. So, meanwhile what
> do
> > > you
> > > > think about this?
> > > 
> > > So, I read both bugs linked and couldn't figure out still why at some
point
> > this
> > > broke? do you have a quick summary? it was working before with weak dumps,
> > so...
> > > what changed?
> > 
> > I have explained in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=661257#c4
> > The dump provider is broken because the global dump ids do not match. I
cannot
> > figure out how to fix this because right now the discardable manager does
not
> > know the process id for the clients. It only knows a client id. It is much
> > effort to figure out how to get pid in the manager now, given that we are
> going
> > to have shared_memory provider which will remove the use of this global dump
> is
> > since it will create the global dump ids. meanwhile when the shared_memory
> > provider is not ready yet, i am just changing the weak dumps to strong ones
> > which will cause only a slight discrepancy in the memory reporting (mostly 0
> in
> > telemetry since we will never hit the case of browser clearing segments
after
> > reaching 500MiB limit).
> 
> A bit more clear: Currently we show 0 in renderer discardable because the
global
> dump ids do not match and all the child discardable dumps get ignored. This cl
> just fixes this ignoring part.

If the IDs don't match won't this cause us to end up with duplicate counting, on
both sides?

[ssid, 2017-02-06 16:02:45]

On 2017/02/06 10:48:16, Primiano Tucci wrote:
> On 2017/02/03 21:59:11, ssid wrote:
> > On 2017/02/03 21:56:32, ssid wrote:
> > > On 2017/02/03 20:09:51, Primiano Tucci wrote:
> > > > On 2017/02/01 03:13:16, ssid wrote:
> > > > > +primiano this is temp fix to not show 0 here. Since it's weak the
dumps
> > > > > disappear now. I can't find anyway to get pid in the manager. We will
> soon
> > > > > remove this code when shared_memory dumps have edges. So, meanwhile
what
> > do
> > > > you
> > > > > think about this?
> > > > 
> > > > So, I read both bugs linked and couldn't figure out still why at some
> point
> > > this
> > > > broke? do you have a quick summary? it was working before with weak
dumps,
> > > so...
> > > > what changed?
> > > 
> > > I have explained in
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=661257#c4
> > > The dump provider is broken because the global dump ids do not match. I
> cannot
> > > figure out how to fix this because right now the discardable manager does
> not
> > > know the process id for the clients. It only knows a client id. It is much
> > > effort to figure out how to get pid in the manager now, given that we are
> > going
> > > to have shared_memory provider which will remove the use of this global
dump
> > is
> > > since it will create the global dump ids. meanwhile when the shared_memory
> > > provider is not ready yet, i am just changing the weak dumps to strong
ones
> > > which will cause only a slight discrepancy in the memory reporting (mostly
0
> > in
> > > telemetry since we will never hit the case of browser clearing segments
> after
> > > reaching 500MiB limit).
> > 
> > A bit more clear: Currently we show 0 in renderer discardable because the
> global
> > dump ids do not match and all the child discardable dumps get ignored. This
cl
> > just fixes this ignoring part.
> 
> If the IDs don't match won't this cause us to end up with duplicate counting,
on
> both sides?

No. We do not add size in browser side dumps. So, the size will only be seen in
renderer. The issue is we cannot match the dumps and find the resident sizes.

[Primiano Tucci (use gerrit), 2017-02-06 18:12:28]

On 2017/02/06 16:02:45, ssid wrote:
> On 2017/02/06 10:48:16, Primiano Tucci wrote:
> > On 2017/02/03 21:59:11, ssid wrote:
> > > On 2017/02/03 21:56:32, ssid wrote:
> > > > On 2017/02/03 20:09:51, Primiano Tucci wrote:
> > > > > On 2017/02/01 03:13:16, ssid wrote:
> > > > > > +primiano this is temp fix to not show 0 here. Since it's weak the
> dumps
> > > > > > disappear now. I can't find anyway to get pid in the manager. We
will
> > soon
> > > > > > remove this code when shared_memory dumps have edges. So, meanwhile
> what
> > > do
> > > > > you
> > > > > > think about this?
> > > > > 
> > > > > So, I read both bugs linked and couldn't figure out still why at some
> > point
> > > > this
> > > > > broke? do you have a quick summary? it was working before with weak
> dumps,
> > > > so...
> > > > > what changed?
> > > > 
> > > > I have explained in
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=661257#c4
> > > > The dump provider is broken because the global dump ids do not match. I
> > cannot
> > > > figure out how to fix this because right now the discardable manager
does
> > not
> > > > know the process id for the clients. It only knows a client id. It is
much
> > > > effort to figure out how to get pid in the manager now, given that we
are
> > > going
> > > > to have shared_memory provider which will remove the use of this global
> dump
> > > is
> > > > since it will create the global dump ids. meanwhile when the
shared_memory
> > > > provider is not ready yet, i am just changing the weak dumps to strong
> ones
> > > > which will cause only a slight discrepancy in the memory reporting
(mostly
> 0
> > > in
> > > > telemetry since we will never hit the case of browser clearing segments
> > after
> > > > reaching 500MiB limit).
> > > 
> > > A bit more clear: Currently we show 0 in renderer discardable because the
> > global
> > > dump ids do not match and all the child discardable dumps get ignored.
This
> cl
> > > just fixes this ignoring part.
> > 
> > If the IDs don't match won't this cause us to end up with duplicate
counting,
> on
> > both sides?
> 
> No. We do not add size in browser side dumps. So, the size will only be seen
in
> renderer. The issue is we cannot match the dumps and find the resident sizes.

I see, LGTM then, but is there  a plan to properly fix this? is the shared mem
dump provider going to solve this?

[ssid, 2017-02-06 19:26:42]

On 2017/02/06 18:12:28, Primiano Tucci wrote:
> On 2017/02/06 16:02:45, ssid wrote:
> > On 2017/02/06 10:48:16, Primiano Tucci wrote:
> > > On 2017/02/03 21:59:11, ssid wrote:
> > > > On 2017/02/03 21:56:32, ssid wrote:
> > > > > On 2017/02/03 20:09:51, Primiano Tucci wrote:
> > > > > > On 2017/02/01 03:13:16, ssid wrote:
> > > > > > > +primiano this is temp fix to not show 0 here. Since it's weak the
> > dumps
> > > > > > > disappear now. I can't find anyway to get pid in the manager. We
> will
> > > soon
> > > > > > > remove this code when shared_memory dumps have edges. So,
meanwhile
> > what
> > > > do
> > > > > > you
> > > > > > > think about this?
> > > > > > 
> > > > > > So, I read both bugs linked and couldn't figure out still why at
some
> > > point
> > > > > this
> > > > > > broke? do you have a quick summary? it was working before with weak
> > dumps,
> > > > > so...
> > > > > > what changed?
> > > > > 
> > > > > I have explained in
> > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=661257#c4
> > > > > The dump provider is broken because the global dump ids do not match.
I
> > > cannot
> > > > > figure out how to fix this because right now the discardable manager
> does
> > > not
> > > > > know the process id for the clients. It only knows a client id. It is
> much
> > > > > effort to figure out how to get pid in the manager now, given that we
> are
> > > > going
> > > > > to have shared_memory provider which will remove the use of this
global
> > dump
> > > > is
> > > > > since it will create the global dump ids. meanwhile when the
> shared_memory
> > > > > provider is not ready yet, i am just changing the weak dumps to strong
> > ones
> > > > > which will cause only a slight discrepancy in the memory reporting
> (mostly
> > 0
> > > > in
> > > > > telemetry since we will never hit the case of browser clearing
segments
> > > after
> > > > > reaching 500MiB limit).
> > > > 
> > > > A bit more clear: Currently we show 0 in renderer discardable because
the
> > > global
> > > > dump ids do not match and all the child discardable dumps get ignored.
> This
> > cl
> > > > just fixes this ignoring part.
> > > 
> > > If the IDs don't match won't this cause us to end up with duplicate
> counting,
> > on
> > > both sides?
> > 
> > No. We do not add size in browser side dumps. So, the size will only be seen
> in
> > renderer. The issue is we cannot match the dumps and find the resident
sizes.
> 
> I see, LGTM then, but is there  a plan to properly fix this? is the shared mem
> dump provider going to solve this?

Yeah with the current plan for shared memory provider this should be solved. If
not we need to find a way to get pid in discardable manager which might be a
battle. I will wait for shared memory provider to be done and decide if we
should get the pid.

[ssid, 2017-02-06 19:27:16]

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

[ssid, 2017-02-06 19:27:16]

+reveman short cl ptal thanks

[reveman, 2017-02-07 03:22:10]

lgtm

[ssid, 2017-02-07 04:12:10]

The CQ bit was checked by ssid@chromium.org

[commit-bot: I haz the power, 2017-02-07 04:12:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 05:30:02]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486440730548560, "parent_rev":
"ca0cadf070a8e3dd09f3f24d6180467b2bc3297b", "commit_rev":
"efed310cc305f0d89f9d1aa49fb5e0d4ec9ff2cd"}

[commit-bot: I haz the power, 2017-02-07 05:31:02]

Description was changed from

==========
[memory-infra] Make client discardable segments non-weak

The discardable segments shared between the processes do not add the
same global dump ids since the process id of the client is not available
in the manager. Temporarily we do not create weak dumps since the dump
provider is broken.

BUG=661257, 687399
==========

to

==========
[memory-infra] Make client discardable segments non-weak

The discardable segments shared between the processes do not add the
same global dump ids since the process id of the client is not available
in the manager. Temporarily we do not create weak dumps since the dump
provider is broken.

BUG=661257, 687399

Review-Url: https://codereview.chromium.org/2668193002
Cr-Commit-Position: refs/heads/master@{#448554}
Committed:
https://chromium.googlesource.com/chromium/src/+/efed310cc305f0d89f9d1aa49fb5...
==========

[commit-bot: I haz the power, 2017-02-07 05:31:03]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/efed310cc305f0d89f9d1aa49fb5...