Use platform sized counters for the refcounted classes in base.

Use platform sized counters for the refcounted classes in base.

Also, fix an implicit cast in OneWriterSeqLockTest.

BUG=629774
Committed: https://crrev.com/c1049960743d2e67130cdef1ab46ef455182b95b
Cr-Commit-Position: refs/heads/master@{#420850}

[Will Harris, 2016-09-20 17:40:15]

Patchset #2 (id:20001) has been deleted

[Will Harris, 2016-09-20 17:46:55]

Description was changed from

==========
Use platform sized counters for the refcounted classes in base.

BUG=629774
==========

to

==========
Use platform sized counters for the refcounted classes in base.

Also, fix an implicit cast in OneWriterSeqLockTest.

BUG=629774
==========

[Will Harris, 2016-09-20 21:33:31]

wfh@chromium.org changed reviewers:
+ thakis@chromium.org

[Will Harris, 2016-09-20 21:33:47]

Thakis, what do you think about this change?

[Will Harris, 2016-09-22 17:13:30]

On 2016/09/20 21:33:47, Will Harris wrote:
> Thakis, what do you think about this change?

ping

[Nico, 2016-09-22 17:15:58]

thakis@chromium.org changed reviewers:
+ tsepez@chromium.org

[Nico, 2016-09-22 17:15:59]

Looks fine to me, but referenced bug is marked WontFix. So I don't know why we'd
want to do this, but I can't think or a reason not to do it either. (Maybe
atomic 64-bit writes are slow on some old 64-bit processors? But that'd be
strange.)

So lgtm if you want to give this a go I guess.

[Will Harris, 2016-09-22 17:28:12]

wfh@chromium.org changed reviewers:
+ creis@chromium.org

[Will Harris, 2016-09-22 17:28:22]

On 2016/09/22 17:15:59, Nico wrote:
> Looks fine to me, but referenced bug is marked WontFix. So I don't know why
we'd
> want to do this, but I can't think or a reason not to do it either. (Maybe
> atomic 64-bit writes are slow on some old 64-bit processors? But that'd be
> strange.)
> 
> So lgtm if you want to give this a go I guess.

Thanks, I updated the bug. I had spoken to the researchers and they made this
suggestion that seemed relatively low cost.

I'll land and see what happens, once I get a content owner +creis.

[Tom Sepez, 2016-09-22 17:50:00]

There's a trade-off here, and the same issue has come up in webkit/blink.

Blink/Webkit are particular about conserving the space, and aren't willing
to spend the extra bytes, knowing that their address space is limited by
PartitonAlloc and we can't fit enough pointers into it to make the count
overflow.

I'm not sure if chrome has the same space concerns.  If so,it's not clear
either that chrome will create enough references to trip this; in particular
the bug is triggered by a test harness binary, not by actually finding a path
in chrome where this can happen.

As a security person, I'm all for this change.

[Tom Sepez, 2016-09-22 17:55:35]

https://codereview.chromium.org/2349393002/diff/40001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/2349393002/diff/40001/base/memory/ref_counted...
base/memory/ref_counted.h:78: mutable size_t ref_count_;
nit: should really be an intprt_t, in case we go back in time to the days of
segemented architectures.

[Charlie Reis, 2016-09-22 18:12:23]

No objection to the test change in content/, but I defer to thakis@/tsepez@ on
the rest.  content/ LGTM.

[Tom Sepez, 2016-09-22 18:46:08]

lgtm

[Will Harris, 2016-09-22 18:47:49]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-09-22 18:48:07]

On 2016/09/22 18:46:08, Tom Sepez wrote:
> lgtm

okay I'll land this and monitor perf bots and see what happens. thanks.

[commit-bot: I haz the power, 2016-09-22 18:48:12]

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, 2016-09-22 20:49:55]

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

[commit-bot: I haz the power, 2016-09-22 20:49:56]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Will Harris, 2016-09-23 20:50:28]

it seems changing a file like this makes the compile times on
linux_chromium_chromeos_rel_ng and linux_chromium_asan_rel_ng so long that they
both timeout before completion. I'm not sure how to proceed on this, so I'll try
and get the current trooper involved.

[Nico, 2016-09-23 21:23:39]

On 2016/09/23 20:50:28, Will Harris wrote:
> it seems changing a file like this makes the compile times on
> linux_chromium_chromeos_rel_ng and linux_chromium_asan_rel_ng so long that
they
> both timeout before completion. I'm not sure how to proceed on this, so I'll
try
> and get the current trooper involved.

You could try sending the try job again, the second time goma will have more
stuff cached and the build will be faster I think

[Will Harris, 2016-09-23 21:25:47]

On 2016/09/23 21:23:39, Nico wrote:
> On 2016/09/23 20:50:28, Will Harris wrote:
> > it seems changing a file like this makes the compile times on
> > linux_chromium_chromeos_rel_ng and linux_chromium_asan_rel_ng so long that
> they
> > both timeout before completion. I'm not sure how to proceed on this, so I'll
> try
> > and get the current trooper involved.
> 
> You could try sending the try job again, the second time goma will have more
> stuff cached and the build will be faster I think

okay I'll try that, but I presume the compile will happen on a different slave
each time, so unless I'm lucky and get the same slave for the second run...

[Nico, 2016-09-23 21:27:00]

The cache is server-side, so as long as you send the retry fairly soon after the
first job I think it might work.

[Will Harris, 2016-09-23 21:28:54]

On 2016/09/23 21:27:00, Nico wrote:
> The cache is server-side, so as long as you send the retry fairly soon after
the
> first job I think it might work.

https://www.google.com/search?q=trying%20the%20same%20thing%20twice%20and%20e...
:) sgtm

[Will Harris, 2016-09-24 00:23:00]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2016-09-24 00:23:35]

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, 2016-09-24 02:40:41]

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

[commit-bot: I haz the power, 2016-09-24 02:40:43]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Will Harris, 2016-09-24 18:12:53]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2016-09-24 18:13:10]

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, 2016-09-24 19:11:48]

Description was changed from

==========
Use platform sized counters for the refcounted classes in base.

Also, fix an implicit cast in OneWriterSeqLockTest.

BUG=629774
==========

to

==========
Use platform sized counters for the refcounted classes in base.

Also, fix an implicit cast in OneWriterSeqLockTest.

BUG=629774
==========

[commit-bot: I haz the power, 2016-09-24 19:11:50]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-24 19:13:21]

Description was changed from

==========
Use platform sized counters for the refcounted classes in base.

Also, fix an implicit cast in OneWriterSeqLockTest.

BUG=629774
==========

to

==========
Use platform sized counters for the refcounted classes in base.

Also, fix an implicit cast in OneWriterSeqLockTest.

BUG=629774

Committed: https://crrev.com/c1049960743d2e67130cdef1ab46ef455182b95b
Cr-Commit-Position: refs/heads/master@{#420850}
==========

[commit-bot: I haz the power, 2016-09-24 19:13:23]

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