Optimize CronetUploadDataStreamAdapter::Read |
|||||||||
Issue descriptionChrome Version: (copy from chrome://version) OS: (e.g. Win7, OSX 10.9.5, etc...) What steps will reproduce the problem? (1) Upload a large(ie: 1GB) file from an Android device using Cronet (2) Record a profile and notice the NewDirectByteBuffer allocation in Read() consumes about 3% of CPU. https://cs.chromium.org/chromium/src/components/cronet/android/cronet_upload_data_stream_adapter.cc?sq=package:chromium&l=50 What is the expected result? A new byte buffer would not be allocated on every read call.
,
Aug 21 2017
I have sent out a CL for review. https://chromium-review.googlesource.com/c/624196
,
Aug 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9580368e08bfba2a3b41f37112c502222f753d20 commit 9580368e08bfba2a3b41f37112c502222f753d20 Author: Helen Li <xunjieli@chromium.org> Date: Mon Aug 21 21:39:08 2017 [cronet] Re-use Direct ByteBuffer for Cronet upload This CL makes Cronet upload reuse a Java ByteBuffer object if the underlying net::IOBuffer's address and buffer length are unchanged. This should reduce the number of constructor calls to NewDirectByteBuffer(). Bug: 756841 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: I751242095e2ba5793750d5a91e2bc3b10ec8b7a1 Reviewed-on: https://chromium-review.googlesource.com/624196 Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org> Commit-Queue: Helen Li <xunjieli@chromium.org> Cr-Commit-Position: refs/heads/master@{#496067} [modify] https://crrev.com/9580368e08bfba2a3b41f37112c502222f753d20/components/cronet/android/cronet_upload_data_stream_adapter.cc [modify] https://crrev.com/9580368e08bfba2a3b41f37112c502222f753d20/components/cronet/android/cronet_upload_data_stream_adapter.h [modify] https://crrev.com/9580368e08bfba2a3b41f37112c502222f753d20/components/cronet/android/io_buffer_with_byte_buffer.cc [modify] https://crrev.com/9580368e08bfba2a3b41f37112c502222f753d20/components/cronet/android/io_buffer_with_byte_buffer.h [modify] https://crrev.com/9580368e08bfba2a3b41f37112c502222f753d20/components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java
,
Aug 31 2017
,
Sep 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9121d1b3a922f380b37bf63fcb0210201131e40 commit f9121d1b3a922f380b37bf63fcb0210201131e40 Author: Helen Li <xunjieli@chromium.org> Date: Thu Sep 07 22:30:28 2017 Revert "[cronet] Re-use Direct ByteBuffer for Cronet upload" This reverts commit 9580368e08bfba2a3b41f37112c502222f753d20. Reason for revert: Suspect that there is a leak in this CL Original change's description: > [cronet] Re-use Direct ByteBuffer for Cronet upload > > This CL makes Cronet upload reuse a Java ByteBuffer object if the > underlying net::IOBuffer's address and buffer length are unchanged. > > This should reduce the number of constructor calls to > NewDirectByteBuffer(). > > Bug: 756841 > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester > Change-Id: I751242095e2ba5793750d5a91e2bc3b10ec8b7a1 > Reviewed-on: https://chromium-review.googlesource.com/624196 > Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org> > Commit-Queue: Helen Li <xunjieli@chromium.org> > Cr-Commit-Position: refs/heads/master@{#496067} TBR=xunjieli@chromium.org,kapishnikov@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 756841 Change-Id: I186de0df7b316e4284648b1b9cca1addc7f7845d Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/656109 Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Helen Li <xunjieli@chromium.org> Cr-Commit-Position: refs/heads/master@{#500411} [modify] https://crrev.com/f9121d1b3a922f380b37bf63fcb0210201131e40/components/cronet/android/cronet_upload_data_stream_adapter.cc [modify] https://crrev.com/f9121d1b3a922f380b37bf63fcb0210201131e40/components/cronet/android/cronet_upload_data_stream_adapter.h [modify] https://crrev.com/f9121d1b3a922f380b37bf63fcb0210201131e40/components/cronet/android/io_buffer_with_byte_buffer.cc [modify] https://crrev.com/f9121d1b3a922f380b37bf63fcb0210201131e40/components/cronet/android/io_buffer_with_byte_buffer.h [modify] https://crrev.com/f9121d1b3a922f380b37bf63fcb0210201131e40/components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java
,
Sep 7 2017
Request a merge of the above revert CL. The CL caused a severe memory leak. I need to merge the revert into M62. Thank you.
,
Sep 7 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/11fcec9ade43e97bab7b0ad37872adcc753cfc29 commit 11fcec9ade43e97bab7b0ad37872adcc753cfc29 Author: Helen Li <xunjieli@chromium.org> Date: Thu Sep 07 22:47:09 2017 Revert "[cronet] Re-use Direct ByteBuffer for Cronet upload" This reverts commit 9580368e08bfba2a3b41f37112c502222f753d20. Reason for revert: Suspect that there is a leak in this CL Original change's description: > [cronet] Re-use Direct ByteBuffer for Cronet upload > > This CL makes Cronet upload reuse a Java ByteBuffer object if the > underlying net::IOBuffer's address and buffer length are unchanged. > > This should reduce the number of constructor calls to > NewDirectByteBuffer(). > > Bug: 756841 > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester > Change-Id: I751242095e2ba5793750d5a91e2bc3b10ec8b7a1 > Reviewed-on: https://chromium-review.googlesource.com/624196 > Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org> > Commit-Queue: Helen Li <xunjieli@chromium.org> > Cr-Commit-Position: refs/heads/master@{#496067} TBR=kapishnikov@chromium.org, xunjieli@chromium.org (cherry picked from commit f9121d1b3a922f380b37bf63fcb0210201131e40) Bug: 756841 Change-Id: I186de0df7b316e4284648b1b9cca1addc7f7845d Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/656109 Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Helen Li <xunjieli@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#500411} Reviewed-on: https://chromium-review.googlesource.com/656002 Cr-Commit-Position: refs/branch-heads/3202@{#72} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/11fcec9ade43e97bab7b0ad37872adcc753cfc29/components/cronet/android/cronet_upload_data_stream_adapter.cc [modify] https://crrev.com/11fcec9ade43e97bab7b0ad37872adcc753cfc29/components/cronet/android/cronet_upload_data_stream_adapter.h [modify] https://crrev.com/11fcec9ade43e97bab7b0ad37872adcc753cfc29/components/cronet/android/io_buffer_with_byte_buffer.cc [modify] https://crrev.com/11fcec9ade43e97bab7b0ad37872adcc753cfc29/components/cronet/android/io_buffer_with_byte_buffer.h [modify] https://crrev.com/11fcec9ade43e97bab7b0ad37872adcc753cfc29/components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java
,
Sep 7 2017
amineer@ told me that there's a M62 dev release today at 5PM PT. Once that's ready, I will do a binary drop.
,
Sep 7 2017
> amineer@ told me that there's a M62 dev release today at 5PM PT. Once that's ready, I will do a binary drop. There will be an M62 *build* today at 5 PM PT. We aren't shipping M62 to the Play Store dev channel until next Tuesday.
,
Sep 7 2017
,
Sep 8 2017
The revert is merged into M62. Re-opening this to target M63.
,
Oct 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd commit d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd Author: Helen Li <xunjieli@chromium.org> Date: Tue Oct 03 16:26:31 2017 [cronet] Reland Re-use Direct ByteBuffer for Cronet upload This is a reland of https://chromium-review.googlesource.com/624196. This CL includes a fix for the Java local ref leak and a regression test. The fix is to wrap NewDirectByteBuffer in a java scoped local ref before passing it to a java scoped global ref. This CL makes Cronet upload reuse a Java ByteBuffer object if the underlying net::IOBuffer's address and buffer length are unchanged. This should reduce the number of constructor calls to NewDirectByteBuffer(). Bug: 756841 Change-Id: I1558d750512f34825604feabab24516c51e58eeb Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/657624 Commit-Queue: Helen Li <xunjieli@chromium.org> Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org> Reviewed-by: Misha Efimov <mef@chromium.org> Cr-Commit-Position: refs/heads/master@{#506064} [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/BUILD.gn [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/api/src/org/chromium/net/UploadDataProvider.java [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/cronet_upload_data_stream_adapter.cc [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/cronet_upload_data_stream_adapter.h [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/io_buffer_with_byte_buffer.cc [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/io_buffer_with_byte_buffer.h [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java [add] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/test/javatests/src/org/chromium/net/CronetStressTest.java [modify] https://crrev.com/d2d86fee1f4e6ca253d02e029d4fe90fde7e26dd/components/cronet/android/test/javatests/src/org/chromium/net/UploadDataProvidersTest.java
,
Oct 3 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by xunji...@chromium.org
, Aug 18 2017