Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815563002:
Avoid cross thread malloc / free pair of IOBuffer on the simple cache (Closed)
DescriptionAvoid cross thread malloc/free pair of IOBuffer on the simple cache
This CL removes a cross thread malloc/free pair from simple disk cache backend.
After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it
destroys |task| part of the given tasks on the target thread, and that
introduced a number of cross thread malloc/free pairs around net::IOBuffer.
The cross thread malloc/free pair increased the apparent size of memory
usage on some Android perf bots by 2~3%, that is likely due to thread
specific free list caches.
BUG=708644
Review-Url: https://codereview.chromium.org/2815563002
Cr-Original-Commit-Position: refs/heads/master@{#464338}
Committed: https://chromium.googlesource.com/chromium/src/+/1714b61635cc8207647b1a9857e8042fbb833025
Review-Url: https://codereview.chromium.org/2815563002
Cr-Commit-Position: refs/heads/master@{#464673}
Committed: https://chromium.googlesource.com/chromium/src/+/248782b9cda664a1ea6d347dd508354d23160a5a
Patch Set 1 #
Total comments: 2
Patch Set 2 : +comment #
Messages
Total messages: 24 (14 generated)
Description was changed from ========== Avoid cross thread malloc / free pair of IOBuffer on the simple cache BUG=708644 ========== to ========== Avoid cross thread malloc/free pair of IOBuffer on the simple cache This CL removes a cross thread malloc/free pair from simple disk cache backend. After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it destroys |task| part of the given tasks on the target thread, and that introduced a number of cross thread malloc/free pairs around net::IOBuffer. The cross thread malloc/free pair increased the apparent size of memory usage on some Android perf bots by 2~3%, that is likely due to thread specific free list caches. BUG=708644 ==========
tzik@chromium.org changed reviewers: + jkarlin@chromium.org, pmeenan@chromium.org, primiano@chromium.org
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
jkarlin: PTAL to the CL itself. pmeenan: FYI. primiano: I think the perf regression alert raised as http://crbug.com/708644 is due to the thread specific free list cache on Android malloc implementation. I'm not sure we should care about it, though this CL seems to get the memory back. Any opinion or advice about this?
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2017/04/11 04:27:49, tzik wrote: > jkarlin: PTAL to the CL itself. > pmeenan: FYI. > > primiano: > I think the perf regression alert raised as http://crbug.com/708644 is due to > the thread specific free list cache on Android malloc implementation. > I'm not sure we should care about it, though this CL seems to get the memory > back. > Any opinion or advice about this? lgtm with a nit. My thought is that if this lands and we verify that it fixes the memory regression then we should revert it and close the bug.
https://codereview.chromium.org/2815563002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/2815563002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_entry_impl.h:255: // Called after an asynchronous write completes. Can you add a comment about why |buf| exists?
On 2017/04/11 04:27:49, tzik wrote: > jkarlin: PTAL to the CL itself. > pmeenan: FYI. > > primiano: > I think the perf regression alert raised as http://crbug.com/708644 is due to > the thread specific free list cache on Android malloc implementation. > I'm not sure we should care about it, though this CL seems to get the memory > back. From https://chromeperf.appspot.com/group_report?bug_id=708644 looks like there are ~1-2 MB regression associated with this. If doing free in the right place fixes that, hey, we should definitely do this :) Thanks a lot for looking into this, your hypothesis is definitely sound.
https://codereview.chromium.org/2815563002/diff/1/net/disk_cache/simple/simpl... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/2815563002/diff/1/net/disk_cache/simple/simpl... net/disk_cache/simple/simple_entry_impl.h:255: // Called after an asynchronous write completes. On 2017/04/11 11:24:39, jkarlin wrote: > Can you add a comment about why |buf| exists? Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2815563002/#ps20001 (title: "+comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492067483822220,
"parent_rev": "998f8e571bebd0ace499f8fef81f91daffada15f", "commit_rev":
"1714b61635cc8207647b1a9857e8042fbb833025"}
Message was sent while issue was closed.
Description was changed from ========== Avoid cross thread malloc/free pair of IOBuffer on the simple cache This CL removes a cross thread malloc/free pair from simple disk cache backend. After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it destroys |task| part of the given tasks on the target thread, and that introduced a number of cross thread malloc/free pairs around net::IOBuffer. The cross thread malloc/free pair increased the apparent size of memory usage on some Android perf bots by 2~3%, that is likely due to thread specific free list caches. BUG=708644 ========== to ========== Avoid cross thread malloc/free pair of IOBuffer on the simple cache This CL removes a cross thread malloc/free pair from simple disk cache backend. After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it destroys |task| part of the given tasks on the target thread, and that introduced a number of cross thread malloc/free pairs around net::IOBuffer. The cross thread malloc/free pair increased the apparent size of memory usage on some Android perf bots by 2~3%, that is likely due to thread specific free list caches. BUG=708644 Review-Url: https://codereview.chromium.org/2815563002 Cr-Commit-Position: refs/heads/master@{#464338} Committed: https://chromium.googlesource.com/chromium/src/+/1714b61635cc8207647b1a9857e8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/1714b61635cc8207647b1a9857e8...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2814743011/ by tzik@chromium.org. The reason for reverting is: Another large memory reduction seems to land in the same batch of the performance dashboard job, and it hid the improvement of this CL. Revert this for now and will reland later..
Message was sent while issue was closed.
Description was changed from ========== Avoid cross thread malloc/free pair of IOBuffer on the simple cache This CL removes a cross thread malloc/free pair from simple disk cache backend. After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it destroys |task| part of the given tasks on the target thread, and that introduced a number of cross thread malloc/free pairs around net::IOBuffer. The cross thread malloc/free pair increased the apparent size of memory usage on some Android perf bots by 2~3%, that is likely due to thread specific free list caches. BUG=708644 Review-Url: https://codereview.chromium.org/2815563002 Cr-Commit-Position: refs/heads/master@{#464338} Committed: https://chromium.googlesource.com/chromium/src/+/1714b61635cc8207647b1a9857e8... ========== to ========== Avoid cross thread malloc/free pair of IOBuffer on the simple cache This CL removes a cross thread malloc/free pair from simple disk cache backend. After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it destroys |task| part of the given tasks on the target thread, and that introduced a number of cross thread malloc/free pairs around net::IOBuffer. The cross thread malloc/free pair increased the apparent size of memory usage on some Android perf bots by 2~3%, that is likely due to thread specific free list caches. BUG=708644 Review-Url: https://codereview.chromium.org/2815563002 Cr-Commit-Position: refs/heads/master@{#464338} Committed: https://chromium.googlesource.com/chromium/src/+/1714b61635cc8207647b1a9857e8... ==========
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492142308601710,
"parent_rev": "16fa46a1db956be40c199add90817ca5ef34a6f0", "commit_rev":
"248782b9cda664a1ea6d347dd508354d23160a5a"}
Message was sent while issue was closed.
Description was changed from ========== Avoid cross thread malloc/free pair of IOBuffer on the simple cache This CL removes a cross thread malloc/free pair from simple disk cache backend. After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it destroys |task| part of the given tasks on the target thread, and that introduced a number of cross thread malloc/free pairs around net::IOBuffer. The cross thread malloc/free pair increased the apparent size of memory usage on some Android perf bots by 2~3%, that is likely due to thread specific free list caches. BUG=708644 Review-Url: https://codereview.chromium.org/2815563002 Cr-Commit-Position: refs/heads/master@{#464338} Committed: https://chromium.googlesource.com/chromium/src/+/1714b61635cc8207647b1a9857e8... ========== to ========== Avoid cross thread malloc/free pair of IOBuffer on the simple cache This CL removes a cross thread malloc/free pair from simple disk cache backend. After a PostTaskAndReply change in http://crrev.com/e5de8d551e80115b, it destroys |task| part of the given tasks on the target thread, and that introduced a number of cross thread malloc/free pairs around net::IOBuffer. The cross thread malloc/free pair increased the apparent size of memory usage on some Android perf bots by 2~3%, that is likely due to thread specific free list caches. BUG=708644 Review-Url: https://codereview.chromium.org/2815563002 Cr-Original-Commit-Position: refs/heads/master@{#464338} Committed: https://chromium.googlesource.com/chromium/src/+/1714b61635cc8207647b1a9857e8... Review-Url: https://codereview.chromium.org/2815563002 Cr-Commit-Position: refs/heads/master@{#464673} Committed: https://chromium.googlesource.com/chromium/src/+/248782b9cda664a1ea6d347dd508... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/248782b9cda664a1ea6d347dd508... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
