Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1429863008:
[Cronet] Remove JSON serialization of CronetEngine.Builder (Closed)
|
Created:
5 years, 1 month ago by pauljensen Modified:
5 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Remove JSON serialization of CronetEngine.Builder
There was no need to use JSON and it was imposing artificial
limits on what we could pass from Java to native.
Instead of JSON, now CronetEngine.Builder configuration is
passed via JNI.
TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig
Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad
Cr-Commit-Position: refs/heads/master@{#363517}
Committed: https://crrev.com/9041eb3c9f2b5b2844c19c5b24ca265e893f7906
Cr-Commit-Position: refs/heads/master@{#364048}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 8
Patch Set 6 : sync #
Total comments: 22
Patch Set 7 : address Helen's comments #
Total comments: 4
Patch Set 8 : sync #Patch Set 9 : remove TextUtils.isEmpty where sensical #
Total comments: 5
Patch Set 10 : sync #Patch Set 11 : address nit #Patch Set 12 : add PKP #
Total comments: 7
Patch Set 13 : address Helen's comments #
Total comments: 6
Patch Set 14 : address Helen's nits #Patch Set 15 : fix component_unittests #Dependent Patchsets: Messages
Total messages: 52 (15 generated)
Description was changed from ========== [Cronet] Remove JSON serialization of CronetEngine.Builder ========== to ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig ==========
pauljensen@chromium.org changed reviewers: + mef@chromium.org
Misha, PTAL. Feel free to delegate review to someone else if you're busy.
mef@chromium.org changed reviewers: + xunjieli@chromium.org
Looks pretty good. Helen, could you please review this? https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:39: // The port on which to use QUIC. Port of the server that supports QUIC. https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:41: // Another port on which to use QUIC. Alternate protocol port. https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/test/cronet_test_jni.cc:31: {"CronetUrlRequestContextConfigText", CronetUrlRequestContextConfigTest (text->test) https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... File components/cronet/android/test/cronet_url_request_context_config_test.h (right): https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/test/cronet_url_request_context_config_test.h:5: #ifndef CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_ Maybe COMPONENTS_CRONET_ANDROID_TEST_CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_?>
My concern is that it will be easy to get parameters mixed up, since the booleans and strings aren't really distinguishable once passed the boundary. Also we are splitting one JNI native call into multiple ones, which I am not sure makes better code. What about passing the booleans, strings, ints using the former approach, and passing MockCertVerifier, Pins (which doesn't make sense to be serialized as ASCII strings) as separate params in the native function (the new approach)? something like: nativeCreateRequestContextAdapter(String config, long MockCertVerifier ...) ?
On 2015/11/10 19:41:01, xunjieli wrote: > My concern is that it will be easy to get parameters mixed up, since the > booleans and strings aren't really distinguishable once passed the boundary. > Also we are splitting one JNI native call into multiple ones, which I am not > sure makes better code. > > What about passing the booleans, strings, ints using the former approach, and > passing MockCertVerifier, Pins (which doesn't make sense to be serialized as > ASCII strings) as separate params in the native function (the new approach)? > > something like: > nativeCreateRequestContextAdapter(String config, long MockCertVerifier ...) ? I'm strongly opposed to a hybrid approach as it'll be quite complicated to maintain: 1. It'll be substantially more code as we'd have to bring back the JSON and the mechanism for having shared strings between Java and native for parsing the string 2. It'll be substantially more complicated to read/explain/grasp. Every time someone wants to add a new field they'll have to determine if they should add it to the string or the other arguments. I also don't see how this greatly improves the chances of not swapping an argument as many of the things not passed via the String will be "long"'s (e.g. mockCertVerifier) which can be swapped inadvertently. I'm not too worried about our ability to get parameters to one particular function call ordered properly as: 1. I imagine any simple test will fail if things are swapped around, 2. We can explicitly test to make sure things are passed through correctly, for example with the test I added.
On 2015/11/10 19:51:50, pauljensen wrote: > On 2015/11/10 19:41:01, xunjieli wrote: > > My concern is that it will be easy to get parameters mixed up, since the > > booleans and strings aren't really distinguishable once passed the boundary. > > Also we are splitting one JNI native call into multiple ones, which I am not > > sure makes better code. > > > > What about passing the booleans, strings, ints using the former approach, and > > passing MockCertVerifier, Pins (which doesn't make sense to be serialized as > > ASCII strings) as separate params in the native function (the new approach)? > > > > something like: > > nativeCreateRequestContextAdapter(String config, long MockCertVerifier ...) ? > > I'm strongly opposed to a hybrid approach as it'll be quite complicated to > maintain: > 1. It'll be substantially more code as we'd have to bring back the JSON and the > mechanism for having shared strings between Java and native for parsing the > string > 2. It'll be substantially more complicated to read/explain/grasp. Every time > someone wants to add a new field they'll have to determine if they should add it > to the string or the other arguments. > I also don't see how this greatly improves the chances of not swapping an > argument as many of the things not passed via the String will be "long"'s (e.g. > mockCertVerifier) which can be swapped inadvertently. > > I'm not too worried about our ability to get parameters to one particular > function call ordered properly as: > 1. I imagine any simple test will fail if things are swapped around, > 2. We can explicitly test to make sure things are passed through correctly, for > example with the test I added. Sorry for the delay. I will take a closer look at this later today or early tomorrow.
On 2015/11/16 18:50:28, xunjieli wrote: > On 2015/11/10 19:51:50, pauljensen wrote: > > On 2015/11/10 19:41:01, xunjieli wrote: > > > My concern is that it will be easy to get parameters mixed up, since the > > > booleans and strings aren't really distinguishable once passed the boundary. > > > Also we are splitting one JNI native call into multiple ones, which I am not > > > sure makes better code. > > > > > > What about passing the booleans, strings, ints using the former approach, > and > > > passing MockCertVerifier, Pins (which doesn't make sense to be serialized as > > > ASCII strings) as separate params in the native function (the new approach)? > > > > > > something like: > > > nativeCreateRequestContextAdapter(String config, long MockCertVerifier ...) > ? > > > > I'm strongly opposed to a hybrid approach as it'll be quite complicated to > > maintain: > > 1. It'll be substantially more code as we'd have to bring back the JSON and > the > > mechanism for having shared strings between Java and native for parsing the > > string > > 2. It'll be substantially more complicated to read/explain/grasp. Every time > > someone wants to add a new field they'll have to determine if they should add > it > > to the string or the other arguments. > > I also don't see how this greatly improves the chances of not swapping an > > argument as many of the things not passed via the String will be "long"'s > (e.g. > > mockCertVerifier) which can be swapped inadvertently. > > > > I'm not too worried about our ability to get parameters to one particular > > function call ordered properly as: > > 1. I imagine any simple test will fail if things are swapped around, > > 2. We can explicitly test to make sure things are passed through correctly, > for > > example with the test I added. > > Sorry for the delay. I will take a closer look at this later today or early > tomorrow. Sorry for the delay. I think this looks good. Could you rebase the changes on ToT? There's the setExperimentalOptions change that needs to be merged.
On 2015/11/30 17:47:40, xunjieli wrote: > On 2015/11/16 18:50:28, xunjieli wrote: > > On 2015/11/10 19:51:50, pauljensen wrote: > > > On 2015/11/10 19:41:01, xunjieli wrote: > > > > My concern is that it will be easy to get parameters mixed up, since the > > > > booleans and strings aren't really distinguishable once passed the > boundary. > > > > Also we are splitting one JNI native call into multiple ones, which I am > not > > > > sure makes better code. > > > > > > > > What about passing the booleans, strings, ints using the former approach, > > and > > > > passing MockCertVerifier, Pins (which doesn't make sense to be serialized > as > > > > ASCII strings) as separate params in the native function (the new > approach)? > > > > > > > > something like: > > > > nativeCreateRequestContextAdapter(String config, long MockCertVerifier > ...) > > ? > > > > > > I'm strongly opposed to a hybrid approach as it'll be quite complicated to > > > maintain: > > > 1. It'll be substantially more code as we'd have to bring back the JSON and > > the > > > mechanism for having shared strings between Java and native for parsing the > > > string > > > 2. It'll be substantially more complicated to read/explain/grasp. Every > time > > > someone wants to add a new field they'll have to determine if they should > add > > it > > > to the string or the other arguments. > > > I also don't see how this greatly improves the chances of not swapping an > > > argument as many of the things not passed via the String will be "long"'s > > (e.g. > > > mockCertVerifier) which can be swapped inadvertently. > > > > > > I'm not too worried about our ability to get parameters to one particular > > > function call ordered properly as: > > > 1. I imagine any simple test will fail if things are swapped around, > > > 2. We can explicitly test to make sure things are passed through correctly, > > for > > > example with the test I added. > > > > Sorry for the delay. I will take a closer look at this later today or early > > tomorrow. > > Sorry for the delay. I think this looks good. Could you rebase the changes on > ToT? There's the setExperimentalOptions change that needs to be merged. Done. PTAL.
https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:39: // The port on which to use QUIC. Could address Misha's comments in Patch #5? https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:41: // Another port on which to use QUIC. Same here. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:58: private String mLibraryName = "cronet"; Why are some fields initialized, and some are not? There are both empty strings and null strings. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:111: nativeAddQuicHint(urlRequestContextConfig, quicHint.mHost, quicHint.mPort, I don't really like adding multiple JNI calls here. Can we consolidate them into some structured info and pass it at once with the rest of the args? https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:429: private static native long nativeCreateRequestContextAdapter(long urlRequestContextConfig); Not sure whether we should consolidate nativeCreateRequestContextContext with nativeCreateRequestContextAdapter, so we have one JNI call. That will sacrifice readability. But the JNI slides do recommend against traversing the boundary unnecessarily. Not sure if this counts. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_test_jni.cc:31: {"CronetUrlRequestContextConfigText", Could you address Misha's comment in Patch 5? CronetUrlRequestContextConfigTest (text->test) https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/test/cronet_url_request_context_config_test.cc (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_url_request_context_config_test.cc:9: #include "base/android/jni_android.h" Need to include base/logging.h for CHECK_EQ https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_url_request_context_config_test.cc:17: // Verify that all the config set by nit: s/Verify/Verifies s/config/configurations https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/test/cronet_url_request_context_config_test.h (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_url_request_context_config_test.h:5: #ifndef CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_ Misha's comment in Patch 5? COMPONENTS_CRONET_ANDROID_TEST_CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_
https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.cc:133: mock_cert_verifier(mock_cert_verifier.Pass()) {} nit: std::move is now preferred over .Pass(). see: scoped_ptr.h I don't have a good understanding of it though. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.h:120: ScopedVector<QuicHint> quic_hints; optional nit (since it is in the original CL), ScopedVector is deprecated. https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... Might consider using std::vector instead.
I addressed or responded to all comments. I'll retest this soon...really wish we had a trybot. https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:39: // The port on which to use QUIC. On 2015/11/09 16:56:33, mef wrote: > Port of the server that supports QUIC. Done. https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:41: // Another port on which to use QUIC. On 2015/11/09 16:56:33, mef wrote: > Alternate protocol port. Done. https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/test/cronet_test_jni.cc:31: {"CronetUrlRequestContextConfigText", On 2015/11/09 16:56:33, mef wrote: > CronetUrlRequestContextConfigTest (text->test) Done. https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... File components/cronet/android/test/cronet_url_request_context_config_test.h (right): https://codereview.chromium.org/1429863008/diff/80001/components/cronet/andro... components/cronet/android/test/cronet_url_request_context_config_test.h:5: #ifndef CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_ On 2015/11/09 16:56:33, mef wrote: > Maybe COMPONENTS_CRONET_ANDROID_TEST_CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_?> Done. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:39: // The port on which to use QUIC. On 2015/12/01 17:30:38, xunjieli wrote: > Could address Misha's comments in Patch #5? Done. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:41: // Another port on which to use QUIC. On 2015/12/01 17:30:38, xunjieli wrote: > Same here. Done. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:58: private String mLibraryName = "cronet"; On 2015/12/01 17:30:38, xunjieli wrote: > Why are some fields initialized, and some are not? > There are both empty strings and null strings. I fixed this. I had avoided doing this to avoid changing several unrelated pieces of code, but I think the change was worth it. I basically changed from .isEmpty() to TextUtils.isEmpty() which takes into account null. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:111: nativeAddQuicHint(urlRequestContextConfig, quicHint.mHost, quicHint.mPort, On 2015/12/01 17:30:38, xunjieli wrote: > I don't really like adding multiple JNI calls here. Can we consolidate them into > some structured info and pass it at once with the rest of the args? I don't really like it either, but I'm not too worried about JNI as the overhead is very low, like around 1 microsecond. I don't like the alternatives any better: 1. could pass an array of classes but we'd have to use GetFieldID() which is horribly slow as it uses reflection. 2. could pass arrays of strings (for the hosts) and arrays of ints (for the ports) but that leads to a hideous looking API and AFAIK there is no handy function for converting from Java arrays of strings to std::vectors of std::strings. I'm open to alternate suggestions. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:429: private static native long nativeCreateRequestContextAdapter(long urlRequestContextConfig); On 2015/12/01 17:30:38, xunjieli wrote: > Not sure whether we should consolidate nativeCreateRequestContextContext with > nativeCreateRequestContextAdapter, so we have one JNI call. That will sacrifice > readability. But the JNI slides do recommend against traversing the boundary > unnecessarily. Not sure if this counts. I split it for a few reasons: 1. The ChromiumURLRequestContext and the CronetURLRequestContext both share the Config JNI (this is why createNativeUrlRequestContextConfig() is package-private not private), but then they create different adapters from the config. 2. the QUIC hints must be added piece by piece in between creating the Config and the Adapter 3. for testing, I want to ensure I create the Config properly but I don't want to have to put in mountains of mocking to actually get an Adapter, I just want to check the Config fields are passed through properly I'm open to alternate suggestions. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/test/cronet_test_jni.cc (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_test_jni.cc:31: {"CronetUrlRequestContextConfigText", On 2015/12/01 17:30:38, xunjieli wrote: > Could you address Misha's comment in Patch 5? > CronetUrlRequestContextConfigTest (text->test) Done. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/test/cronet_url_request_context_config_test.cc (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_url_request_context_config_test.cc:9: #include "base/android/jni_android.h" On 2015/12/01 17:30:38, xunjieli wrote: > Need to include base/logging.h for CHECK_EQ Done. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_url_request_context_config_test.cc:17: // Verify that all the config set by On 2015/12/01 17:30:38, xunjieli wrote: > nit: s/Verify/Verifies Done. > s/config/configurations It only tests one configuration so I don't want to pluralize that, but added the plural "options" afterwards. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... File components/cronet/android/test/cronet_url_request_context_config_test.h (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/andr... components/cronet/android/test/cronet_url_request_context_config_test.h:5: #ifndef CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_ On 2015/12/01 17:30:38, xunjieli wrote: > Misha's comment in Patch 5? > COMPONENTS_CRONET_ANDROID_TEST_CRONET_URL_REQUEST_CONTEXT_CONFIG_TEST_H_ Done. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.cc:133: mock_cert_verifier(mock_cert_verifier.Pass()) {} On 2015/12/01 20:40:20, xunjieli wrote: > nit: std::move is now preferred over .Pass(). > see: scoped_ptr.h > > I don't have a good understanding of it though. Done. https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1429863008/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.h:120: ScopedVector<QuicHint> quic_hints; On 2015/12/01 20:40:20, xunjieli wrote: > optional nit (since it is in the original CL), ScopedVector is deprecated. > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... > > Might consider using std::vector instead. Let's leave that for another CL, especially because I find ScopedVector<QuicHint> more readable than std::vector<scoped_ptr<QuicHint>>.
I'd really like to know if there's any performance impact. It should be pretty straightforward to measure. We can invoke thousands calls of nativeCreateRequestContextAdapter(createNativeUrlRequestContextConfig(builder)) and get the average. Then compare that with nativeCreateRequestContextAdapter(builder.toJSONString()); in the old code. The call can be made multiple times, since CreateRequestContextAdapter creates the builder, but does not actually build it. We can comment out the thread creation in CronetRequestContextAdapter's constructor, so we don't have thousands of threads (since that's not relevant). Just want to make sure we are not in the dark about the performance benefits. I will sign off after this :) https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: if (TextUtils.isEmpty(storagePath())) { I don't understand why we need TextUtils.isEmpty. If storagePath is set, then it is not null. Can we test null instead? https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:693: if (TextUtils.isEmpty(builder.getUserAgent())) { Same here. Can we do null test? If the embedder wants an empty string as the user agent, we should probably allow it, since that is done by an explicit user action.
On 2015/12/02 13:06:46, xunjieli wrote: > I'd really like to know if there's any performance impact. It should be pretty > straightforward to measure. > > We can invoke thousands calls of > nativeCreateRequestContextAdapter(createNativeUrlRequestContextConfig(builder)) > and get the average. Then compare that with > nativeCreateRequestContextAdapter(builder.toJSONString()); in the old code. The > call can be made multiple times, since CreateRequestContextAdapter creates the > builder, but does not actually build it. We can comment out the thread creation > in CronetRequestContextAdapter's constructor, so we don't have thousands of > threads (since that's not relevant). > > Just want to make sure we are not in the dark about the performance benefits. I > will sign off after this :) > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > File components/cronet/android/api/src/org/chromium/net/CronetEngine.java > (right): > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: if > (TextUtils.isEmpty(storagePath())) { > I don't understand why we need TextUtils.isEmpty. If storagePath is set, then it > is not null. Can we test null instead? > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:693: if > (TextUtils.isEmpty(builder.getUserAgent())) { > Same here. Can we do null test? If the embedder wants an empty string as the > user agent, we should probably allow it, since that is done by an explicit user > action. So here's my microbenchmark: diff --git a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java index fc2f6a1..dbab6be 100644 --- a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java +++ b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java @@ -154,7 +154,19 @@ public class CronetSampleActivity extends Activity { .enableHTTP2(true) .enableQUIC(true); - mCronetEngine = myBuilder.build(); + long startTime = System.nanoTime(); + long minTime = 9999999; + for (int i = 0; i < 1000; i++) { + long sTime = System.nanoTime(); + mCronetEngine = myBuilder.build(); + long eTime = System.nanoTime(); + eTime -= sTime; + if(eTime < minTime) minTime = eTime; + mCronetEngine.shutdown(); + } + long endTime = System.nanoTime(); + Log.e(TAG, "avg CronetEngine build: " + ((endTime - startTime) / 100) + " ns"); + Log.e(TAG, "min CronetEngine build: " + minTime + " ns"); And here are the results without my patch: avg CronetEngine build: 48924942 ns min CronetEngine build: 966823 ns avg CronetEngine build: 49502997 ns min CronetEngine build: 970104 ns avg CronetEngine build: 51697553 ns min CronetEngine build: 1040417 ns avg CronetEngine build: 53930207 ns min CronetEngine build: 1022812 ns And here are the results with my patch: avg CronetEngine build: 47030748 ns min CronetEngine build: 849688 ns avg CronetEngine build: 46135060 ns min CronetEngine build: 839427 ns avg CronetEngine build: 48232359 ns min CronetEngine build: 864063 ns avg CronetEngine build: 44086663 ns min CronetEngine build: 843490 ns I think the "min" times are the important ones because they aren't biased significantly from things like loading libcronet.so, and they don't take into account shutdown() time. So CronetEngine.Builder.build() went from ~1ms to ~0.85ms, a 15% improvement. I think the improvement will grow over time as using JNI directly allow more freedom, for example Andrei's cert pinning CL won't have to base64 encode binary things and then decode them.
On 2015/12/02 17:13:04, pauljensen wrote: > On 2015/12/02 13:06:46, xunjieli wrote: > > I'd really like to know if there's any performance impact. It should be pretty > > straightforward to measure. > > > > We can invoke thousands calls of > > > nativeCreateRequestContextAdapter(createNativeUrlRequestContextConfig(builder)) > > and get the average. Then compare that with > > nativeCreateRequestContextAdapter(builder.toJSONString()); in the old code. > The > > call can be made multiple times, since CreateRequestContextAdapter creates the > > builder, but does not actually build it. We can comment out the thread > creation > > in CronetRequestContextAdapter's constructor, so we don't have thousands of > > threads (since that's not relevant). > > > > Just want to make sure we are not in the dark about the performance benefits. > I > > will sign off after this :) > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > File components/cronet/android/api/src/org/chromium/net/CronetEngine.java > > (right): > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: if > > (TextUtils.isEmpty(storagePath())) { > > I don't understand why we need TextUtils.isEmpty. If storagePath is set, then > it > > is not null. Can we test null instead? > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:693: if > > (TextUtils.isEmpty(builder.getUserAgent())) { > > Same here. Can we do null test? If the embedder wants an empty string as the > > user agent, we should probably allow it, since that is done by an explicit > user > > action. > > So here's my microbenchmark: > diff --git > a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > index fc2f6a1..dbab6be 100644 > --- > a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > +++ > b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > @@ -154,7 +154,19 @@ public class CronetSampleActivity extends Activity { > .enableHTTP2(true) > .enableQUIC(true); > > - mCronetEngine = myBuilder.build(); > + long startTime = System.nanoTime(); > + long minTime = 9999999; > + for (int i = 0; i < 1000; i++) { > + long sTime = System.nanoTime(); > + mCronetEngine = myBuilder.build(); > + long eTime = System.nanoTime(); > + eTime -= sTime; > + if(eTime < minTime) minTime = eTime; > + mCronetEngine.shutdown(); > + } > + long endTime = System.nanoTime(); > + Log.e(TAG, "avg CronetEngine build: " + ((endTime - startTime) / 100) + > " ns"); > + Log.e(TAG, "min CronetEngine build: " + minTime + " ns"); > > > And here are the results without my patch: > avg CronetEngine build: 48924942 ns > min CronetEngine build: 966823 ns > avg CronetEngine build: 49502997 ns > min CronetEngine build: 970104 ns > avg CronetEngine build: 51697553 ns > min CronetEngine build: 1040417 ns > avg CronetEngine build: 53930207 ns > min CronetEngine build: 1022812 ns > And here are the results with my patch: > avg CronetEngine build: 47030748 ns > min CronetEngine build: 849688 ns > avg CronetEngine build: 46135060 ns > min CronetEngine build: 839427 ns > avg CronetEngine build: 48232359 ns > min CronetEngine build: 864063 ns > avg CronetEngine build: 44086663 ns > min CronetEngine build: 843490 ns > > I think the "min" times are the important ones because they aren't biased > significantly from things like loading libcronet.so, and they don't take into > account shutdown() time. > So CronetEngine.Builder.build() went from ~1ms to ~0.85ms, a 15% improvement. > I think the improvement will grow over time as using JNI directly allow more > freedom, for example Andrei's cert pinning CL won't have to base64 encode binary > things and then decode them. I think saving of .15ms is fairly noble, although ideally it should only happen a few times per life cycle of the app. On this note I wonder whether we should add a counter of CronetEngines being built and throw an exception like 'Thou shall not create an engine for every request' if it exceeds certain limit.
On 2015/12/02 17:46:43, mef wrote: > On 2015/12/02 17:13:04, pauljensen wrote: > > On 2015/12/02 13:06:46, xunjieli wrote: > > > I'd really like to know if there's any performance impact. It should be > pretty > > > straightforward to measure. > > > > > > We can invoke thousands calls of > > > > > > nativeCreateRequestContextAdapter(createNativeUrlRequestContextConfig(builder)) > > > and get the average. Then compare that with > > > nativeCreateRequestContextAdapter(builder.toJSONString()); in the old code. > > The > > > call can be made multiple times, since CreateRequestContextAdapter creates > the > > > builder, but does not actually build it. We can comment out the thread > > creation > > > in CronetRequestContextAdapter's constructor, so we don't have thousands of > > > threads (since that's not relevant). > > > > > > Just want to make sure we are not in the dark about the performance > benefits. > > I > > > will sign off after this :) > > > > > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > > File components/cronet/android/api/src/org/chromium/net/CronetEngine.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: if > > > (TextUtils.isEmpty(storagePath())) { > > > I don't understand why we need TextUtils.isEmpty. If storagePath is set, > then > > it > > > is not null. Can we test null instead? > > > > > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:693: if > > > (TextUtils.isEmpty(builder.getUserAgent())) { > > > Same here. Can we do null test? If the embedder wants an empty string as the > > > user agent, we should probably allow it, since that is done by an explicit > > user > > > action. > > > > So here's my microbenchmark: > > diff --git > > > a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > > b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > index fc2f6a1..dbab6be 100644 > > --- > > > a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > +++ > > > b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > @@ -154,7 +154,19 @@ public class CronetSampleActivity extends Activity { > > .enableHTTP2(true) > > .enableQUIC(true); > > > > - mCronetEngine = myBuilder.build(); > > + long startTime = System.nanoTime(); > > + long minTime = 9999999; > > + for (int i = 0; i < 1000; i++) { > > + long sTime = System.nanoTime(); > > + mCronetEngine = myBuilder.build(); > > + long eTime = System.nanoTime(); > > + eTime -= sTime; > > + if(eTime < minTime) minTime = eTime; > > + mCronetEngine.shutdown(); > > + } > > + long endTime = System.nanoTime(); > > + Log.e(TAG, "avg CronetEngine build: " + ((endTime - startTime) / 100) > + > > " ns"); > > + Log.e(TAG, "min CronetEngine build: " + minTime + " ns"); > > > > > > And here are the results without my patch: > > avg CronetEngine build: 48924942 ns > > min CronetEngine build: 966823 ns > > avg CronetEngine build: 49502997 ns > > min CronetEngine build: 970104 ns > > avg CronetEngine build: 51697553 ns > > min CronetEngine build: 1040417 ns > > avg CronetEngine build: 53930207 ns > > min CronetEngine build: 1022812 ns > > And here are the results with my patch: > > avg CronetEngine build: 47030748 ns > > min CronetEngine build: 849688 ns > > avg CronetEngine build: 46135060 ns > > min CronetEngine build: 839427 ns > > avg CronetEngine build: 48232359 ns > > min CronetEngine build: 864063 ns > > avg CronetEngine build: 44086663 ns > > min CronetEngine build: 843490 ns > > > > I think the "min" times are the important ones because they aren't biased > > significantly from things like loading libcronet.so, and they don't take into > > account shutdown() time. > > So CronetEngine.Builder.build() went from ~1ms to ~0.85ms, a 15% improvement. > > I think the improvement will grow over time as using JNI directly allow more > > freedom, for example Andrei's cert pinning CL won't have to base64 encode > binary > > things and then decode them. > > I think saving of .15ms is fairly noble, although ideally it should only happen > a few times per life cycle of the app. > On this note I wonder whether we should add a counter of CronetEngines being > built and throw an exception like 'Thou shall not create an engine for every > request' if it exceeds certain limit. Having a hidden counter that makes your app crash at a seemingly random time in the future seems like a bad idea.
On 2015/12/02 17:46:43, mef wrote: > On 2015/12/02 17:13:04, pauljensen wrote: > > On 2015/12/02 13:06:46, xunjieli wrote: > > > I'd really like to know if there's any performance impact. It should be > pretty > > > straightforward to measure. > > > > > > We can invoke thousands calls of > > > > > > nativeCreateRequestContextAdapter(createNativeUrlRequestContextConfig(builder)) > > > and get the average. Then compare that with > > > nativeCreateRequestContextAdapter(builder.toJSONString()); in the old code. > > The > > > call can be made multiple times, since CreateRequestContextAdapter creates > the > > > builder, but does not actually build it. We can comment out the thread > > creation > > > in CronetRequestContextAdapter's constructor, so we don't have thousands of > > > threads (since that's not relevant). > > > > > > Just want to make sure we are not in the dark about the performance > benefits. > > I > > > will sign off after this :) > > > > > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > > File components/cronet/android/api/src/org/chromium/net/CronetEngine.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: if > > > (TextUtils.isEmpty(storagePath())) { > > > I don't understand why we need TextUtils.isEmpty. If storagePath is set, > then > > it > > > is not null. Can we test null instead? > > > > > > > > > https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... > > > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:693: if > > > (TextUtils.isEmpty(builder.getUserAgent())) { > > > Same here. Can we do null test? If the embedder wants an empty string as the > > > user agent, we should probably allow it, since that is done by an explicit > > user > > > action. > > > > So here's my microbenchmark: > > diff --git > > > a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > > b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > index fc2f6a1..dbab6be 100644 > > --- > > > a/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > +++ > > > b/components/cronet/android/sample/src/org/chromium/cronet_sample_apk/CronetSampleActivity.java > > @@ -154,7 +154,19 @@ public class CronetSampleActivity extends Activity { > > .enableHTTP2(true) > > .enableQUIC(true); > > > > - mCronetEngine = myBuilder.build(); > > + long startTime = System.nanoTime(); > > + long minTime = 9999999; > > + for (int i = 0; i < 1000; i++) { > > + long sTime = System.nanoTime(); > > + mCronetEngine = myBuilder.build(); > > + long eTime = System.nanoTime(); > > + eTime -= sTime; > > + if(eTime < minTime) minTime = eTime; > > + mCronetEngine.shutdown(); > > + } > > + long endTime = System.nanoTime(); > > + Log.e(TAG, "avg CronetEngine build: " + ((endTime - startTime) / 100) > + > > " ns"); > > + Log.e(TAG, "min CronetEngine build: " + minTime + " ns"); > > > > > > And here are the results without my patch: > > avg CronetEngine build: 48924942 ns > > min CronetEngine build: 966823 ns > > avg CronetEngine build: 49502997 ns > > min CronetEngine build: 970104 ns > > avg CronetEngine build: 51697553 ns > > min CronetEngine build: 1040417 ns > > avg CronetEngine build: 53930207 ns > > min CronetEngine build: 1022812 ns > > And here are the results with my patch: > > avg CronetEngine build: 47030748 ns > > min CronetEngine build: 849688 ns > > avg CronetEngine build: 46135060 ns > > min CronetEngine build: 839427 ns > > avg CronetEngine build: 48232359 ns > > min CronetEngine build: 864063 ns > > avg CronetEngine build: 44086663 ns > > min CronetEngine build: 843490 ns > > > > I think the "min" times are the important ones because they aren't biased > > significantly from things like loading libcronet.so, and they don't take into > > account shutdown() time. > > So CronetEngine.Builder.build() went from ~1ms to ~0.85ms, a 15% improvement. > > I think the improvement will grow over time as using JNI directly allow more > > freedom, for example Andrei's cert pinning CL won't have to base64 encode > binary > > things and then decode them. > > I think saving of .15ms is fairly noble, although ideally it should only happen > a few times per life cycle of the app. > On this note I wonder whether we should add a counter of CronetEngines being > built and throw an exception like 'Thou shall not create an engine for every > request' if it exceeds certain limit. Also, for your microbenchmark it would be interesting to track the first engine creation separately to better understand cold start scenario.
https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: if (TextUtils.isEmpty(storagePath())) { On 2015/12/02 13:06:46, xunjieli wrote: > I don't understand why we need TextUtils.isEmpty. If storagePath is set, then it > is not null. Can we test null instead? Done. I was using TextUtils.isEmpty() to match the old code (which uses .isEmpty()) but that is no longer necessary. https://codereview.chromium.org/1429863008/diff/120001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:693: if (TextUtils.isEmpty(builder.getUserAgent())) { On 2015/12/02 13:06:46, xunjieli wrote: > Same here. Can we do null test? If the embedder wants an empty string as the > user agent, we should probably allow it, since that is done by an explicit user > action. Done. I was using TextUtils.isEmpty() to match the old code (which uses .isEmpty()) but that is no longer necessary.
lgtm with two more nits. Thanks for running the benchmark. The numbers are very convincing! https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:309: if (storagePath() == null) { nit: since we are already inside the builder, can we use mStoragePath directly (here and the next if-statement? Just to be consistent with other variables in this method https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:314: throw new IllegalArgumentException("Storage path must be empty"); nit: maybe change "Storage path must be empty" -> "Storage path must not be set"
https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:309: if (storagePath() == null) { On 2015/12/02 18:55:35, xunjieli wrote: > nit: since we are already inside the builder, can we use mStoragePath directly > (here and the next if-statement? Just to be consistent with other variables in > this method enableHttpCache() is more-or-less a setter for mDisableCache and mHttpCacheMode so it should certainly be accessing them directly. mStoragePath versus storagePath() I think is more of a style choice. Some people think member methods should always use accessors and others don't. I checked all the style guides I know of and none seemed to express an opinion. Personally I like always using setters and getters so as to preserve the abstraction layer. I think I'll leave this as is unless you feel strongly (which I assume not as this was a nit). https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:314: throw new IllegalArgumentException("Storage path must be empty"); On 2015/12/02 18:55:35, xunjieli wrote: > nit: maybe change "Storage path must be empty" -> "Storage path must not be set" Done.
https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1429863008/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:309: if (storagePath() == null) { On 2015/12/03 18:51:45, pauljensen wrote: > On 2015/12/02 18:55:35, xunjieli wrote: > > nit: since we are already inside the builder, can we use mStoragePath directly > > (here and the next if-statement? Just to be consistent with other variables in > > this method > > enableHttpCache() is more-or-less a setter for mDisableCache and mHttpCacheMode > so it should certainly be accessing them directly. mStoragePath versus > storagePath() I think is more of a style choice. Some people think member > methods should always use accessors and others don't. I checked all the style > guides I know of and none seemed to express an opinion. Personally I like > always using setters and getters so as to preserve the abstraction layer. I > think I'll leave this as is unless you feel strongly (which I assume not as this > was a nit). sgtm since it isn't a new change.
PTAL, I sync'ed past Andrei's PKP CL (https://codereview.chromium.org/1407263010/) and my net::HashValue CL (https://codereview.chromium.org/1432663002/) and then reworked the PKP stuff to pass the values in binary format. Sorry there is no intermediate "sync" patchset, there were a lot of merge conflicts so it wouldn't have been all that useful. You can look at diffs with patchset #10 if you like.
https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:598: jlong jexpiration_time) { Maybe add a comment here documenting what |jexpiration_time| is. Something like number of milliseconds since the epoch. https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:609: if (env->GetArrayLength(bytes_array.obj()) != sizeof(net::SHA256HashValue)) Does this actually work? The size of the struct includes all of its members right? Should it be: net::SHA256HashValue hash_value; if (env->GetArrayLength(bytes_array.obj()) != sizeof(hash_value.data)) https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:612: net::HashValue hash(*reinterpret_cast<net::SHA256HashValue*>(bytes)); SHA256HashValue is a struct. Does the reinterpret_cast actually work? Should we allocate a SHA256HashValue on the stack, and do a memcpy of the bytes as unint_8/unsigned char to the SHA256HashValue's data field? https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:433: private static native void nativeAddPkp(long urlRequestContextConfig, String host, nit: Need to document what |expirationTime| is. Took me a while to understand why you are adding Epoch on top of expirationTime
https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:598: jlong jexpiration_time) { On 2015/12/04 19:19:13, xunjieli wrote: > Maybe add a comment here documenting what |jexpiration_time| is. Something like > number of milliseconds since the epoch. Done. https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:609: if (env->GetArrayLength(bytes_array.obj()) != sizeof(net::SHA256HashValue)) On 2015/12/04 19:19:13, xunjieli wrote: > Does this actually work? The size of the struct includes all of its members > right? > > Should it be: > net::SHA256HashValue hash_value; > > if (env->GetArrayLength(bytes_array.obj()) != sizeof(hash_value.data)) Yes, this works. In fact SHA256HashValue exists to be a POD representation of the hash value, see paragraph two here: https://code.google.com/p/chromium/issues/detail?id=551067#c2 I've added a static_assert to enforce SHA256HashValue is POD and contains no overhead. https://codereview.chromium.org/1429863008/diff/220001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:612: net::HashValue hash(*reinterpret_cast<net::SHA256HashValue*>(bytes)); On 2015/12/04 19:19:13, xunjieli wrote: > SHA256HashValue is a struct. Does the reinterpret_cast actually work? > > Should we allocate a SHA256HashValue on the stack, and do a memcpy of the bytes > as unint_8/unsigned char to the SHA256HashValue's data field? > Yes, this works. I really want to avoid memcpy as it's very type-unsafe and add a useless copy. I've added a static_assert to enforce SHA256HashValue is POD and contains no overhead.
lgtm with nits https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:591: // Add a public key ping to URLRequestContextConfig. nit: s/ping/pin https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:596: // Array of byte[32] representing SHA256 key hashes. nit: I haven't seen this style of comments. Maybe move to the top of this method. Something like: // Add a public key ping to URLRequestContextConfig. // |jhashes| is an array of jbyte[32] representing SHA256 key hashes. // |jexpiration_time| is the time that the pin expires, in milliseconds since Jan. 1, 1970, midnight GMT. https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:616: if (env->GetArrayLength(bytes_array.obj()) != sizeof(net::SHA256HashValue)) Andrei's patch as a LOG statement. Maybe consider adding that back? DVLOG(WARNING) << "Unable to add public key hash value.";
https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:591: // Add a public key ping to URLRequestContextConfig. On 2015/12/07 15:28:02, xunjieli wrote: > nit: s/ping/pin Done. https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:596: // Array of byte[32] representing SHA256 key hashes. On 2015/12/07 15:28:02, xunjieli wrote: > nit: I haven't seen this style of comments. Maybe move to the top of this > method. Something like: > > // Add a public key ping to URLRequestContextConfig. > // |jhashes| is an array of jbyte[32] representing SHA256 key hashes. > // |jexpiration_time| is the time that the pin expires, in milliseconds since > Jan. 1, 1970, midnight GMT. Done. https://codereview.chromium.org/1429863008/diff/240001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:616: if (env->GetArrayLength(bytes_array.obj()) != sizeof(net::SHA256HashValue)) On 2015/12/07 15:28:02, xunjieli wrote: > Andrei's patch as a LOG statement. Maybe consider adding that back? > DVLOG(WARNING) << "Unable to add public key hash value."; Done, upgraded to LOG(ERROR) as this is a serious internal condition.
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1429863008/#ps260001 (title: "address Helen's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429863008/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429863008/260001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig ========== to ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig ========== to ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1507783002/ by caitkp@chromium.org. The reason for reverting is: Looks like this broke the android build: https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus... FAILED: /b/build/goma/gomacc /b/build/slave/Android_Debug__Nexus_9_/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/components/cronet/cronet_unittests.url_request_context_config_unittest.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DCR_CLANG_REVISION=254049-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_BROWSER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DDONT_EMBED_BUILD_METADATA -DFIELDTRIAL_TESTING_ENABLED -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_SUPERVISED_USERS=1 -DVIDEO_HOLE=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_WEBVR -DSAFE_BROWSING_DB_REMOTE -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=0 -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DUNIT_TEST -DGTEST_HAS_RTTI=0 -DGTEST_USE_OWN_TR1_TUPLE=1 -DGTEST_HAS_TR1_TUPLE=1 -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DANDROID -D__GNU_SOURCE=1 '-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -Igen -Igen/protoc_out -I../.. -I../../third_party/protobuf -I../../third_party/protobuf/src -I../../testing/gtest/include -fstack-protector --param=ssp-buffer-size=4 -Werror -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wno-unused-local-typedefs -march=armv7-a -mtune=generic-armv7-a -mfpu=vfpv3-d16 -mfloat-abi=softfp -mthumb -fno-tree-sra -fno-caller-saves -Wno-psabi -mthumb-interwork -ffunction-sections -funwind-tables -g -fstack-protector -fno-short-enums -finline-limit=64 --sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libcxx/include -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../../third_party/android_tools/ndk//sources/android/support/include -Os -g -fdata-sections -ffunction-sections -fomit-frame-pointer -funwind-tables -g0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -Wno-abi -std=gnu++11 -Wno-narrowing -Wno-literal-suffix -c ../../components/cronet/url_request_context_config_unittest.cc -o obj/components/cronet/cronet_unittests.url_request_context_config_unittest.o ../../components/cronet/url_request_context_config_unittest.cc: In member function 'virtual void cronet::URLRequestContextConfigTest_SetQuicExperimentalOptions_Test::TestBody()': ../../components/cronet/url_request_context_config_unittest.cc:17:27: error: no matching function for call to 'cronet::URLRequestContextConfig::URLRequestContextConfig()' URLRequestContextConfig config; ^ ../../components/cronet/url_request_context_config_unittest.cc:17:27: note: candidates are: In file included from ../../components/cronet/url_request_context_config.h:10:0, from ../../components/cronet/url_request_context_config_unittest.cc:5: ../../components/cronet/url_request_context_config.h:148:28: note: cronet::URLRequestContextConfig::URLRequestContextConfig(const cronet::URLRequestContextConfig&) DISALLOW_COPY_AND_ASSIGN(URLRequestContextConfig); ^ ../../base/macros.h:27:3: note: in definition of macro 'DISALLOW_COPY_AND_ASSIGN' TypeName(const TypeName&); \ ^ ../../components/cronet/url_request_context_config.h:148:28: note: candidate expects 1 argument, 0 provided DISALLOW_COPY_AND_ASSIGN(URLRequestContextConfig); ^ ../../base/macros.h:27:3: note: in definition of macro 'DISALLOW_COPY_AND_ASSIGN' TypeName(const TypeName&); \ ^ In file included from ../../components/cronet/url_request_context_config_unittest.cc:5:0: ../../components/cronet/url_request_context_config.h:72:3: note: cronet::URLRequestContextConfig::URLRequestContextConfig(bool, bool, bool, cronet::URLRequestContextConfig::HttpCacheType, int, bool, const string&, const string&, const string&, const string&, const string&, const string&, const string&, scoped_ptr<net::CertVerifier>) URLRequestContextConfig( ^ ../../components/cronet/url_request_context_config.h:72:3: note: candidate expects 14 arguments, 0 provided ../../components/cronet/url_request_context_config_unittest.cc:35:10: error: 'struct cronet::URLRequestContextConfig' has no member named 'LoadFromJSON' config.LoadFromJSON(args); ^ ninja: build stopped: subcommand failed..
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517} ========== to ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517} ==========
Helen, PTAL @ diffs versus PS#14
On 2015/12/08 12:37:56, pauljensen wrote: > Helen, PTAL @ diffs versus PS#14 lgtm. I totally forgot about the unit test. sorry about that.
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429863008/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429863008/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429863008/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429863008/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429863008/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429863008/280001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517} ========== to ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517} ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517} ========== to ========== [Cronet] Remove JSON serialization of CronetEngine.Builder There was no need to use JSON and it was imposing artificial limits on what we could pass from Java to native. Instead of JSON, now CronetEngine.Builder configuration is passed via JNI. TEST=CronetUrlRequestContextTest.testCronetEngineBuilderConfig Committed: https://crrev.com/2ab237105cf00f7d82c1006798fdf0359a38a3ad Cr-Commit-Position: refs/heads/master@{#363517} Committed: https://crrev.com/9041eb3c9f2b5b2844c19c5b24ca265e893f7906 Cr-Commit-Position: refs/heads/master@{#364048} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9041eb3c9f2b5b2844c19c5b24ca265e893f7906 Cr-Commit-Position: refs/heads/master@{#364048} |
