Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968453002:
Eliminate copying of data in iOS protocol handler to improve performance (Closed)
|
Created:
3 years, 5 months ago by kapishnikov Modified:
3 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, ios-reviews_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEliminate copying of data in iOS protocol handler to improve performance
This change:
1. Eliminates data copying between URLRequest and the NSURLSession
client buffers.
2. Dynamically changes the size of the read buffer based on the size of
previously read data.
3. Account for the case when [NSInputStream:read] returns -1, that
indicates an error.
Some internal profiling tests show approx 2% overall network thread
improvement with this change.
BUG=724546
Review-Url: https://codereview.chromium.org/2968453002
Cr-Commit-Position: refs/heads/master@{#483705}
Committed: https://chromium.googlesource.com/chromium/src/+/4bca026aca0b992f6cad31d794db2102ac7c353e
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments #Patch Set 3 : More static_casts #
Total comments: 6
Patch Set 4 : Addressed comments #Patch Set 5 : Changed ERR_UNEXPECTED to ERR_FAILED #
Total comments: 2
Patch Set 6 : Comment fixes. #
Total comments: 2
Patch Set 7 : Changed operator 'new' to 'malloc' #Messages
Total messages: 19 (6 generated)
kapishnikov@chromium.org changed reviewers: + droger@chromium.org, mef@chromium.org
These are some iOS protocol handler optimizations. PTAL.
looks pretty good. I agree that POST optimizations deserve separate CL. https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... ios/net/crn_http_protocol_handler.mm:264: read:(unsigned char*)(read_buffer_.get())maxLength:read_buffer_size_]; nit: weird formatting. Maybe try git cl fomat ios/net? https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... ios/net/crn_http_protocol_handler.mm:265: if (length) { Length could be -1 if error occurred. We should handle that.
https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... ios/net/crn_http_protocol_handler.mm:263: length = [(NSInputStream*)stream drive-by: style guide bans casts using C style (https://google.github.io/styleguide/cppguide.html#Casting). Since you are already updating this file, would you mind converting them to C++ cast, and ObjCCastStrict (from base/mac/foundation_util.h)? length = [base::max::ObjCCastStrict<NSInputStream>(stream) read:static_cast<unsigned char*>(buffer_->data()) maxLength:read_buffer_size_]; Note that you can remove the DCHECK if you use ObjCCastStrict as it uses -isKindOfClass:.
Description was changed from ========== Eliminate copying of data in iOS protocol handler to improve performance This change: 1. Eliminates data copying between URLRequest and the NSURLSession client buffers. 2. Dynamically changes the size of the read buffer based on the size of previously read data. Some internal profiling tests show approx 2% overall network thread improvement with this change. BUG=724546 ========== to ========== Eliminate copying of data in iOS protocol handler to improve performance This change: 1. Eliminates data copying between URLRequest and the NSURLSession client buffers. 2. Dynamically changes the size of the read buffer based on the size of previously read data. 3. Account for the case when [NSInputStream:read] returns -1, that indicates an error. Some internal profiling tests show approx 2% overall network thread improvement with this change. BUG=724546 ==========
Thanks for the review comment. https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... ios/net/crn_http_protocol_handler.mm:263: length = [(NSInputStream*)stream On 2017/06/29 16:32:53, sdefresne wrote: > drive-by: style guide bans casts using C style > (https://google.github.io/styleguide/cppguide.html#Casting). Since you are > already updating this file, would you mind converting them to C++ cast, and > ObjCCastStrict (from base/mac/foundation_util.h)? > > length = [base::max::ObjCCastStrict<NSInputStream>(stream) > read:static_cast<unsigned char*>(buffer_->data()) > maxLength:read_buffer_size_]; > > Note that you can remove the DCHECK if you use ObjCCastStrict as it uses > -isKindOfClass:. Done. It is good to know. Thanks! https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... ios/net/crn_http_protocol_handler.mm:264: read:(unsigned char*)(read_buffer_.get())maxLength:read_buffer_size_]; On 2017/06/29 16:18:38, mef wrote: > nit: weird formatting. Maybe try git cl fomat ios/net? That was the output of "git cl format". Agree that it is strange. https://codereview.chromium.org/2968453002/diff/1/ios/net/crn_http_protocol_h... ios/net/crn_http_protocol_handler.mm:265: if (length) { On 2017/06/29 16:18:38, mef wrote: > Length could be -1 if error occurred. We should handle that. Nice catch. Fixed it.
https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:272: StopRequestWithError(stream.streamError.code, ERR_UNEXPECTED); nit: I'd argue that this is ERR_FAILED, as it is not caused by caused by a programming mistake or an invalid assumption. https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:512: read_buffer_wrapper_ = Should this |read_buffer_wrapper_| be created in constructor? This will closer match existing state. https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:554: read_buffer_wrapper_ = Should AllocateReadBuffer also create read_buffer_wrapper_? This may make it easier to understand.
https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:272: StopRequestWithError(stream.streamError.code, ERR_UNEXPECTED); On 2017/06/29 20:52:55, mef wrote: > nit: I'd argue that this is ERR_FAILED, as it is not caused by caused by a > programming mistake or an invalid assumption. Done. https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:512: read_buffer_wrapper_ = On 2017/06/29 20:52:55, mef wrote: > Should this |read_buffer_wrapper_| be created in constructor? This will closer > match existing state. Done. https://codereview.chromium.org/2968453002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:554: read_buffer_wrapper_ = On 2017/06/29 20:52:55, mef wrote: > Should AllocateReadBuffer also create read_buffer_wrapper_? > This may make it easier to understand. Good point. Done.
lgtm https://codereview.chromium.org/2968453002/diff/80001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/80001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:540: // The NSData will take the ownership of |buffer_|. nit: buffer_ -> read_buffer_
https://codereview.chromium.org/2968453002/diff/80001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/80001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:540: // The NSData will take the ownership of |buffer_|. On 2017/06/29 22:14:08, mef wrote: > nit: buffer_ -> read_buffer_ Done.
lgtm https://codereview.chromium.org/2968453002/diff/100001/ios/net/crn_http_proto... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/100001/ios/net/crn_http_proto... ios/net/crn_http_protocol_handler.mm:581: read_buffer_.reset(new char[read_buffer_size_]); The doc of dataWithBytesNoCopy specifies that the memory has to be allocated with malloc. Is it ok to call new here?
https://codereview.chromium.org/2968453002/diff/100001/ios/net/crn_http_proto... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2968453002/diff/100001/ios/net/crn_http_proto... ios/net/crn_http_protocol_handler.mm:581: read_buffer_.reset(new char[read_buffer_size_]); On 2017/06/30 13:07:09, droger wrote: > The doc of dataWithBytesNoCopy specifies that the memory has to be allocated > with malloc. Is it ok to call new here? That is a great catch! I think we should stick to the documentation regardless. I will change it to malloc. Thanks!
On 2017/06/30 13:25:37, kapishnikov wrote: > https://codereview.chromium.org/2968453002/diff/100001/ios/net/crn_http_proto... > File ios/net/crn_http_protocol_handler.mm (right): > > https://codereview.chromium.org/2968453002/diff/100001/ios/net/crn_http_proto... > ios/net/crn_http_protocol_handler.mm:581: read_buffer_.reset(new > char[read_buffer_size_]); > On 2017/06/30 13:07:09, droger wrote: > > The doc of dataWithBytesNoCopy specifies that the memory has to be allocated > > with malloc. Is it ok to call new here? > > That is a great catch! I think we should stick to the documentation regardless. > I will change it to malloc. Thanks! Done.
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, droger@chromium.org Link to the patchset: https://codereview.chromium.org/2968453002/#ps120001 (title: "Changed operator 'new' to 'malloc'")
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": 120001, "attempt_start_ts": 1498834059081370,
"parent_rev": "80ec8c4dc77b19abc1a3c04f438ca369092a2a85", "commit_rev":
"4bca026aca0b992f6cad31d794db2102ac7c353e"}
Message was sent while issue was closed.
Description was changed from ========== Eliminate copying of data in iOS protocol handler to improve performance This change: 1. Eliminates data copying between URLRequest and the NSURLSession client buffers. 2. Dynamically changes the size of the read buffer based on the size of previously read data. 3. Account for the case when [NSInputStream:read] returns -1, that indicates an error. Some internal profiling tests show approx 2% overall network thread improvement with this change. BUG=724546 ========== to ========== Eliminate copying of data in iOS protocol handler to improve performance This change: 1. Eliminates data copying between URLRequest and the NSURLSession client buffers. 2. Dynamically changes the size of the read buffer based on the size of previously read data. 3. Account for the case when [NSInputStream:read] returns -1, that indicates an error. Some internal profiling tests show approx 2% overall network thread improvement with this change. BUG=724546 Review-Url: https://codereview.chromium.org/2968453002 Cr-Commit-Position: refs/heads/master@{#483705} Committed: https://chromium.googlesource.com/chromium/src/+/4bca026aca0b992f6cad31d794db... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4bca026aca0b992f6cad31d794db... |
