[iOS Cronet]: Eliminate an extra thread hop when calling NSURLProtocolClient from HttpProtocolHandlerCore |
||||
Issue descriptionHttpProtocolHandlerCore implementation uses CRNHTTPProtocolHandlerProxyWithClientThread as a proxy to invoke NSURLProtocolClient methods. The proxy doesn't invoke the methods directly but instead posting them on a client (CFNetwork.CustomProtocol) thread. Profiling shows that invoking the methods in such manner may contribute to more than 3% of the overall network thread CPU utilization. There is no documented reason why an implementer of HttpProtocolHandler should not invoke NSURLProtocolClient methods directly. It would be reasonable to assume that if that was the case, the protocol handler API would provide a dispatch_queue to use rather than asking the implementer to call [NSThread currentThread]. According to profiling, calling NSURLProtocolClient directly is very lightweight operation that internally posts the tasks to an internal execution queue controlled by CFNetwork. Calling the NSURLProtocolClient directly reduces the network thread CPU utilization by around 2% and decreases the load on CFNetwork.CustomProtocol thread. All the above was tested on iOS 10.3. It is possible that previous versions of iOS had a bug that lead to crashes when NSURLProtocolClient was called directly. Therefore, any changes to the code should be tested on as many different iOS versions as possible.
,
Jul 8 2017
You understood the bug correctly. That is the link I was looking for. I am surprised that it is implemented that way. Currently, the most expensive operations in onReadCompleted() are: 54.2% -[NSObject(NSThreadPerformAdditions) performSelector:onThread:withObject:waitUntilDone:modes:] 23.3% -[NSObject(NSObject) methodSignatureForSelector:] Can we somehow get rid of "performSelector:onThread:" method? I have attached a screenshot of the profiling results when NSURLProtocolClient is called directly. It can be seen that the client basically executes a single "addBlockAndSignal" operation. I don't know exactly what it does, but I would assume that it posts a task (block) on an internal dispatch_queue.
,
Jul 19 2017
Assigning to kapishnikov@ since it seems to be a tracking bug.
,
Aug 3 2017
I have sent for review a CL with the change recommended by Rob: https://chromium-review.googlesource.com/c/600112
,
Aug 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c98f1e5b89e705d5b25b3870e7ca995ca27c16b9 commit c98f1e5b89e705d5b25b3870e7ca995ca27c16b9 Author: kapishnikov <kapishnikov@chromium.org> Date: Thu Aug 03 20:46:42 2017 Improve performance of CRNHTTPProtocolHandlerProxyWithClientThread by switching to blocks The change improves the performance of CRNHTTPProtocolHandlerProxyWithClientThread and simplifies the code. Profiling indicates that the new postBlockToClientThread method is about 37% faster than the removed postToClientThread method. Bug: 740139 Change-Id: Ib35e17f1c04318845bc1a1e4cb19aa0876023845 Reviewed-on: https://chromium-review.googlesource.com/600112 Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org> Reviewed-by: Misha Efimov <mef@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#491838} [modify] https://crrev.com/c98f1e5b89e705d5b25b3870e7ca995ca27c16b9/ios/net/crn_http_protocol_handler_proxy_with_client_thread.mm
,
Aug 3 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by robgaunt@google.com
, Jul 8 2017