|
|
Created:
4 years, 3 months ago by Will Harris Modified:
4 years, 2 months ago CC:
chromium-reviews, gavinp+memory_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse 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}
Patch Set 1 #Patch Set 2 : fix implicit cast in seqlock test #
Total comments: 1
Messages
Total messages: 33 (12 generated)
Patchset #2 (id:20001) has been deleted
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 ==========
wfh@chromium.org changed reviewers: + thakis@chromium.org
Thakis, what do you think about this change?
On 2016/09/20 21:33:47, Will Harris wrote: > Thakis, what do you think about this change? ping
thakis@chromium.org changed reviewers: + tsepez@chromium.org
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.
wfh@chromium.org changed reviewers: + creis@chromium.org
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.
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.
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.
No objection to the test change in content/, but I defer to thakis@/tsepez@ on the rest. content/ LGTM.
lgtm
The CQ bit was checked by wfh@chromium.org
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.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
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
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...
The cache is server-side, so as long as you send the retry fairly soon after the first job I think it might work.
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
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c1049960743d2e67130cdef1ab46ef455182b95b Cr-Commit-Position: refs/heads/master@{#420850} |