Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875083002:
Add url_request_quic_perftest.cc (Closed)
DescriptionAdd url_request_quic_perftest.cc
This CL adds a simple perf test that checks the finish time of running
1000 requests against a HTTP/1.1 server that advertises QUIC support on
an alternative host. This test also checks the end state of //net
MemoryDumpProvider to make sure that there are no leftover
HttpStreamFactoryImpl::Jobs or Quic Jobs.
Follow-up CLs will add more complex tests cases.
BUG=701387
Review-Url: https://codereview.chromium.org/2875083002
Cr-Commit-Position: refs/heads/master@{#477001}
Committed: https://chromium.googlesource.com/chromium/src/+/cc6b1d0925c81c56615211252251707a25214a6c
Patch Set 1 : self #Patch Set 2 : fix deps #
Total comments: 9
Patch Set 3 : address comments #
Total comments: 5
Patch Set 4 : Use final result from MemoryDumpManager #
Total comments: 12
Patch Set 5 : address comment #Patch Set 6 : address ned's comment #Patch Set 7 : address primiano comment to use TraceAnalyzer #Patch Set 8 : self #Patch Set 9 : Rebased #Messages
Total messages: 58 (39 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== temp do not review BUG= ========== to ========== Add a url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG= ==========
Description was changed from ========== Add a url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG= ========== to ========== Add a url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG= ==========
Description was changed from ========== Add a url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG= ========== to ========== Add url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG= ==========
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Add url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG= ========== to ========== Add url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG=701387 ==========
xunjieli@chromium.org changed reviewers: + rch@chromium.org, zhongyi@chromium.org
Ryan and Cherie: This is the test that I mentioned in an email a while ago. My main goal is to have integration tests running on Telemetry that can track memory usage for core //net use cases. There are limits to what these tests can do, so if this turns out not to be useful, I will remove the code. PTAL. Thanks!
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Cool! https://codereview.chromium.org/2875083002/diff/140001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2875083002/diff/140001/net/BUILD.gn#newcode3055 net/BUILD.gn:3055: source_set("quic_test_tools") { I'm sure this makes sense, but it's not obvious why it's required?
nednguyen@google.com changed reviewers: + nednguyen@google.com, primiano@chromium.org
Primiano: can you take a pass at reviewing how memory data are collected in micro benchmark here? https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:190: PrintPerfTest("time", (end - start).InMilliseconds(), "count"); umm, shouldn't the unit here be "ms"? Also what is the noise level you are seeing in this? Why not divide this duration by 1000 above to get the "average_round_trip" time? https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:195: std::unique_ptr<base::trace_event::ProcessMemoryDump> process_memory_dump( not sure this is the good API to get memory dump in a micro benchmark. +Primiano: thoughts? https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:233: base::JSONWriter::Write(*raw_attrs.get(), &json); I think we shouldn't need to manually do these json parsing just to query these data :) https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:240: ASSERT_TRUE(base::HexStringToInt(object_count_str, &object_count)); This is a mixed of perf tests & correctness test?
Thanks! https://codereview.chromium.org/2875083002/diff/140001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2875083002/diff/140001/net/BUILD.gn#newcode3055 net/BUILD.gn:3055: source_set("quic_test_tools") { On 2017/05/25 22:03:15, Ryan Hamilton wrote: > I'm sure this makes sense, but it's not obvious why it's required? Sorry, I should have included a note. I extracted all quic test util files from "net_unittests" into a separate target, because the "net_perftests" also uses these util files. For example, quic/test_tools/crypto_test_utils.h is used for test::crypto_test_utils::ProofSourceForTesting() in instantiating a QuicSimpleServer. https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:190: PrintPerfTest("time", (end - start).InMilliseconds(), "count"); On 2017/05/25 23:57:40, nednguyen wrote: > umm, shouldn't the unit here be "ms"? Also what is the noise level you are > seeing in this? Why not divide this duration by 1000 above to get the > "average_round_trip" time? Done. The result ranges from 3600ms - 4500ms for these 1000 requests on my linux machine. Dividing by 1000, my test runs report 3ms or 4ms. https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:233: base::JSONWriter::Write(*raw_attrs.get(), &json); On 2017/05/25 23:57:40, nednguyen wrote: > I think we shouldn't need to manually do these json parsing just to query these > data :) This JSON parsing was mostly for debugging. Removed. https://codereview.chromium.org/2875083002/diff/140001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:240: ASSERT_TRUE(base::HexStringToInt(object_count_str, &object_count)); On 2017/05/25 23:57:40, nednguyen wrote: > This is a mixed of perf tests & correctness test? Yes. I used ASSERT for fatal errors -- when we failed to get data out of ProcessMemoryDump/AllocatorDump. I used EXPECT for correctness -- we should not be seeing |active_jobs|/|active_quic_jobs| and more than 1 |all_sessions|. I am planning to track total malloc, but I am not sure if there's an API for that.
Is there any chance you can just turn trace (TraceLog::SetEnabled(), see MemoryDumpManagerTest) and look at the final trace result using base/test/trace_event_analyzer.h ? Those methods are not meant to be used outside of MDM and concretely we are going to refactor that stuff for the memory-uma v2 project so that would end up making the refactoring more difficult.
https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:234: ASSERT_TRUE(base::HexStringToInt(object_count_str, &object_count)); If this test can be enable on CQ, why not change this to ASSERT so that we will block CQ if people create memory leaks? Do you think it could be flaky?
> Is there any chance you can just turn trace (TraceLog::SetEnabled(), see > MemoryDumpManagerTest) and look at the final trace result using > base/test/trace_event_analyzer.h ? > Those methods are not meant to be used outside of MDM and concretely we are > going to refactor that stuff for the memory-uma v2 project so that would end up > making the refactoring more difficult. Thanks! I will try using base/test/trace_event_analyzer.h instead. Will update the thread when I have a new patch. https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:234: ASSERT_TRUE(base::HexStringToInt(object_count_str, &object_count)); On 2017/05/26 18:38:57, nednguyen wrote: > If this test can be enable on CQ, why not change this to ASSERT so that we will > block CQ if people create memory leaks? > > Do you think it could be flaky? Isn't this already an ASSERT? It shouldn't be flaky.
https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:234: ASSERT_TRUE(base::HexStringToInt(object_count_str, &object_count)); On 2017/05/26 19:13:41, xunjieli wrote: > On 2017/05/26 18:38:57, nednguyen wrote: > > If this test can be enable on CQ, why not change this to ASSERT so that we > will > > block CQ if people create memory leaks? > > > > Do you think it could be flaky? > > Isn't this already an ASSERT? It shouldn't be flaky. Ooops, wrong line. I mean the line that does: "EXPECT_EQ("0", object_count_str);" I assume you want to assert object_count always equal 0?
Patchset #4 (id:180001) has been deleted
Thanks! The CL is ready for reviewing. PTAL.
I used primiano@'s MemoryDumpManagerTest example and added:
PrintPerfTest("chrome_malloc_total_kb", result.chrome_dump.malloc_total_kb,
"kB");
PrintPerfTest("os_resident_set_kb", result.os_dump.resident_set_kb, "kB");
https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_quic_perftest.cc (right):
https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re...
net/url_request/url_request_quic_perftest.cc:234:
ASSERT_TRUE(base::HexStringToInt(object_count_str, &object_count));
On 2017/05/26 19:15:52, nednguyen wrote:
> On 2017/05/26 19:13:41, xunjieli wrote:
> > On 2017/05/26 18:38:57, nednguyen wrote:
> > > If this test can be enable on CQ, why not change this to ASSERT so that we
> > will
> > > block CQ if people create memory leaks?
> > >
> > > Do you think it could be flaky?
> >
> > Isn't this already an ASSERT? It shouldn't be flaky.
>
> Ooops, wrong line. I mean the line that does: "EXPECT_EQ("0",
> object_count_str);" I assume you want to assert object_count always equal 0?
I used EXPECT_EQ() there instead of ASSERT_EQ() because I wanted the test to
finish executing. I think EXPECT failures will result in the test failing. The
effect should be the same, but in the case of EXPECT, I can get more debug data
out of the test if it fails. I can change it to ASSERT if you feel strongly
about it.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/160001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:234: ASSERT_TRUE(base::HexStringToInt(object_count_str, &object_count)); On 2017/05/26 23:57:16, xunjieli wrote: > On 2017/05/26 19:15:52, nednguyen wrote: > > On 2017/05/26 19:13:41, xunjieli wrote: > > > On 2017/05/26 18:38:57, nednguyen wrote: > > > > If this test can be enable on CQ, why not change this to ASSERT so that we > > > will > > > > block CQ if people create memory leaks? > > > > > > > > Do you think it could be flaky? > > > > > > Isn't this already an ASSERT? It shouldn't be flaky. > > > > Ooops, wrong line. I mean the line that does: "EXPECT_EQ("0", > > object_count_str);" I assume you want to assert object_count always equal 0? > > I used EXPECT_EQ() there instead of ASSERT_EQ() because I wanted the test to > finish executing. I think EXPECT failures will result in the test failing. The > effect should be the same, but in the case of EXPECT, I can get more debug data > out of the test if it fails. I can change it to ASSERT if you feel strongly > about it. Ah I see, as long as the final return code is non zero in case EXPECT fails, it's good. Sorry I haven't been coding C++ for a while https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:178: quic_server_->Shutdown(); what about shutting down tcp_server_? https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:216: MemoryTestURLRequestContext* context() const { return context_.get(); } why defining this if we can just use context_-> instead? https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:309: EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _)) silly question, what is this mock test for?
Patchset #5 (id:220001) has been deleted
Thanks! https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:178: quic_server_->Shutdown(); On 2017/05/27 00:28:39, nednguyen wrote: > what about shutting down tcp_server_? The destructor of EmbeddedTestServer calls ShutdownAndWaitUntilComplete(). I think it's okay not to call it explicitly. https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:216: MemoryTestURLRequestContext* context() const { return context_.get(); } On 2017/05/27 00:28:39, nednguyen wrote: > why defining this if we can just use context_-> instead? It's hard to reason about the tests if the tests start to modifying more member variables. Ideally we should hide those member variables from the tests and provide accessors for those that the tests need. I uploaded a new patch to do the same for |global_dump_handler_| and |mdm_|, so hopefully it's more consistent? https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:309: EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _)) On 2017/05/27 00:28:39, nednguyen wrote: > silly question, what is this mock test for? I am following the MemoryDumpManagerTest example. This handler adds a RequestGlobalDumpFunction to intercept any request for a global dump, so it can create a process dump with a custom callback to record the results (in ProcessDumpRecordingCallbackAdapter).
Thanks for bearing with my questions. I have no ownership, so feel free to skip me if you have net OWNERs & primiano's stamp. :-) https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:178: quic_server_->Shutdown(); On 2017/05/29 20:16:00, xunjieli wrote: > On 2017/05/27 00:28:39, nednguyen wrote: > > what about shutting down tcp_server_? > > The destructor of EmbeddedTestServer calls ShutdownAndWaitUntilComplete(). > I think it's okay not to call it explicitly. I see. Can you add comment explaining why we don't need to explicitly shutdown tcp_sever_? Just curious, why doesn't QuickSever also call shutdown in destructor? https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:216: MemoryTestURLRequestContext* context() const { return context_.get(); } On 2017/05/29 20:16:00, xunjieli wrote: > On 2017/05/27 00:28:39, nednguyen wrote: > > why defining this if we can just use context_-> instead? > > It's hard to reason about the tests if the tests start to modifying more member > variables. Ideally we should hide those member variables from the tests and > provide accessors for those that the tests need. I uploaded a new patch to do > the same for |global_dump_handler_| and |mdm_|, so hopefully it's more > consistent? I think if the accessor just returns the non-const version of the member, then it's essentially the same as accessing them directly. I do some code search and see other unittests in Chromium also access private member directly: https://cs.chromium.org/chromium/src/base/metrics/histogram_snapshot_manager_... IIRC, the test class is instantiated once per test method, so we need not to worry about all test methods mutating its state differently. Having said those, I don't code in Chrome C++ codebase much, so I defer to other reviewers for the final call. https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:309: EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _)) On 2017/05/29 20:16:00, xunjieli wrote: > On 2017/05/27 00:28:39, nednguyen wrote: > > silly question, what is this mock test for? > > I am following the MemoryDumpManagerTest example. This handler adds a > RequestGlobalDumpFunction to intercept any request for a global dump, so it can > create a process dump with a custom callback to record the results (in > ProcessDumpRecordingCallbackAdapter). It seems a bit unintuitive to me to rely on gtest EXPECT_CALL to just intercept the request a global dump. However, if primiano@ thinks this is the supported way, I am ok :)
xunjieli@chromium.org changed reviewers: - nednguyen@google.com, zhongyi@chromium.org
Primiano: PTAL. Thank you. https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... File net/url_request/url_request_quic_perftest.cc (right): https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:178: quic_server_->Shutdown(); On 2017/05/30 13:31:38, nednguyen wrote: > On 2017/05/29 20:16:00, xunjieli wrote: > > On 2017/05/27 00:28:39, nednguyen wrote: > > > what about shutting down tcp_server_? > > > > The destructor of EmbeddedTestServer calls ShutdownAndWaitUntilComplete(). > > I think it's okay not to call it explicitly. > > I see. Can you add comment explaining why we don't need to explicitly shutdown > tcp_sever_? Done. > > Just curious, why doesn't QuickSever also call shutdown in destructor? I am not sure either :) https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:216: MemoryTestURLRequestContext* context() const { return context_.get(); } On 2017/05/30 13:31:38, nednguyen wrote: > On 2017/05/29 20:16:00, xunjieli wrote: > > On 2017/05/27 00:28:39, nednguyen wrote: > > > why defining this if we can just use context_-> instead? > > > > It's hard to reason about the tests if the tests start to modifying more > member > > variables. Ideally we should hide those member variables from the tests and > > provide accessors for those that the tests need. I uploaded a new patch to do > > the same for |global_dump_handler_| and |mdm_|, so hopefully it's more > > consistent? > > I think if the accessor just returns the non-const version of the member, then > it's essentially the same as accessing them directly. I do some code search and > see other unittests in Chromium also access private member directly: > https://cs.chromium.org/chromium/src/base/metrics/histogram_snapshot_manager_... > > IIRC, the test class is instantiated once per test method, so we need not to > worry about all test methods mutating its state differently. > > Having said those, I don't code in Chrome C++ codebase much, so I defer to other > reviewers for the final call. I hear you, but I find it easier to reason about test logic when members have explicit accessors. This way, when people add more member fields, the members are default to private. https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... net/url_request/url_request_quic_perftest.cc:309: EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _)) On 2017/05/30 13:31:38, nednguyen wrote: > On 2017/05/29 20:16:00, xunjieli wrote: > > On 2017/05/27 00:28:39, nednguyen wrote: > > > silly question, what is this mock test for? > > > > I am following the MemoryDumpManagerTest example. This handler adds a > > RequestGlobalDumpFunction to intercept any request for a global dump, so it > can > > create a process dump with a custom callback to record the results (in > > ProcessDumpRecordingCallbackAdapter). > > It seems a bit unintuitive to me to rely on gtest EXPECT_CALL to just intercept > the request a global dump. > However, if primiano@ thinks this is the supported way, I am ok :) Acknowledged.
On 2017/06/02 20:44:00, xunjieli wrote: > Primiano: PTAL. Thank you. > > https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... > File net/url_request/url_request_quic_perftest.cc (right): > > https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... > net/url_request/url_request_quic_perftest.cc:178: quic_server_->Shutdown(); > On 2017/05/30 13:31:38, nednguyen wrote: > > On 2017/05/29 20:16:00, xunjieli wrote: > > > On 2017/05/27 00:28:39, nednguyen wrote: > > > > what about shutting down tcp_server_? > > > > > > The destructor of EmbeddedTestServer calls ShutdownAndWaitUntilComplete(). > > > I think it's okay not to call it explicitly. > > > > I see. Can you add comment explaining why we don't need to explicitly shutdown > > tcp_sever_? > > Done. > > > > Just curious, why doesn't QuickSever also call shutdown in destructor? > > I am not sure either :) > > https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... > net/url_request/url_request_quic_perftest.cc:216: MemoryTestURLRequestContext* > context() const { return context_.get(); } > On 2017/05/30 13:31:38, nednguyen wrote: > > On 2017/05/29 20:16:00, xunjieli wrote: > > > On 2017/05/27 00:28:39, nednguyen wrote: > > > > why defining this if we can just use context_-> instead? > > > > > > It's hard to reason about the tests if the tests start to modifying more > > member > > > variables. Ideally we should hide those member variables from the tests and > > > provide accessors for those that the tests need. I uploaded a new patch to > do > > > the same for |global_dump_handler_| and |mdm_|, so hopefully it's more > > > consistent? > > > > I think if the accessor just returns the non-const version of the member, then > > it's essentially the same as accessing them directly. I do some code search > and > > see other unittests in Chromium also access private member directly: > > > https://cs.chromium.org/chromium/src/base/metrics/histogram_snapshot_manager_... > > > > IIRC, the test class is instantiated once per test method, so we need not to > > worry about all test methods mutating its state differently. > > > > Having said those, I don't code in Chrome C++ codebase much, so I defer to > other > > reviewers for the final call. > > I hear you, but I find it easier to reason about test logic when members have > explicit accessors. This way, when people add more member fields, the members > are default to private. > > https://codereview.chromium.org/2875083002/diff/200001/net/url_request/url_re... > net/url_request/url_request_quic_perftest.cc:309: > EXPECT_CALL(global_dump_handler_, RequestGlobalMemoryDump(_, _)) > On 2017/05/30 13:31:38, nednguyen wrote: > > On 2017/05/29 20:16:00, xunjieli wrote: > > > On 2017/05/27 00:28:39, nednguyen wrote: > > > > silly question, what is this mock test for? > > > > > > I am following the MemoryDumpManagerTest example. This handler adds a > > > RequestGlobalDumpFunction to intercept any request for a global dump, so it > > can > > > create a process dump with a custom callback to record the results (in > > > ProcessDumpRecordingCallbackAdapter). > > > > It seems a bit unintuitive to me to rely on gtest EXPECT_CALL to just > intercept > > the request a global dump. > > However, if primiano@ thinks this is the supported way, I am ok :) > > Acknowledged. My original suggestion above was to not depend on any of this (ReqGlobalDump, MemoryAllocatorDump, attributes_for_Testing, etc) and just use tracing end-to-end and then use TraceAnalyzer to read back the JSON. Quoting from #25: """ Is there any chance you can just turn trace (TraceLog::SetEnabled(), see MemoryDumpManagerTest) and look at the final trace result using base/test/trace_event_analyzer.h ? Those methods are not meant to be used outside of MDM and concretely we are going to refactor that stuff for the memory-uma v2 project so that would end up making the refactoring more difficult. """ I think you just ended up increasing the dependencies on the internals here, which is a lose-lose in terms of code maintenance for both of us :) Sorry I just realized I might have explained myself poorly in the suggestion above. My original proposal was: use only TraceLog::SetEnabled/Disabled (like you do) and MDM::CreateProcessDump (it will bypass the multi-process logic that requires all that boilerplate including the EXPECT_CALL stuff). Read back the trace JSON using TraceAnalyzer. Advantage of this: you will not depend on any memory-infra internals (get_attributes_for_testing(), the TracedValue serialization, yada yada) but only on the final trace json, which is de facto a quite stable API surface these days. Disadvantage: using TraceAnalyzer to get the data back might be a bit more code than today's attributes_for_testing(). but at the same time you'll get rid of all the existing boilerplate for ReqGlobalDump This should allow you to avoid most of the boilerplate introduced here. If for any reason this shouldn't JustWork, I don't want to block this at this point I'd prefer going back to your original proposal (just invoke OnMemoryDump on a mock MAD, without using tracelog nor MDM). Happy to IM if there is any doubt to get this unblocked
> My original suggestion above was to not depend on any of this (ReqGlobalDump, > MemoryAllocatorDump, attributes_for_Testing, etc) and just use tracing > end-to-end and then use TraceAnalyzer to read back the JSON. > Quoting from #25: > """ > Is there any chance you can just turn trace (TraceLog::SetEnabled(), see > MemoryDumpManagerTest) and look at the final trace result using > base/test/trace_event_analyzer.h ? > Those methods are not meant to be used outside of MDM and concretely we are > going to refactor that stuff for the memory-uma v2 project so that would end up > making the refactoring more difficult. > """ > I think you just ended up increasing the dependencies on the internals here, > which is a lose-lose in terms of code maintenance for both of us :) > Sorry I just realized I might have explained myself poorly in the suggestion > above. > > My original proposal was: use only TraceLog::SetEnabled/Disabled (like you do) > and MDM::CreateProcessDump (it will bypass the multi-process logic that requires > all that boilerplate including the EXPECT_CALL stuff). Read back the trace JSON > using TraceAnalyzer. > Advantage of this: you will not depend on any memory-infra internals > (get_attributes_for_testing(), the TracedValue serialization, yada yada) but > only on the final trace json, which is de facto a quite stable API surface these > days. > Disadvantage: using TraceAnalyzer to get the data back might be a bit more code > than today's attributes_for_testing(). but at the same time you'll get rid of > all the existing boilerplate for ReqGlobalDump > > This should allow you to avoid most of the boilerplate introduced here. > > If for any reason this shouldn't JustWork, I don't want to block this at this > point I'd prefer going back to your original proposal (just invoke OnMemoryDump > on a mock MAD, without using tracelog nor MDM). > Happy to IM if there is any doubt to get this unblocked Done. PTAL? I misinterpreted your suggestion. Ned was asking if there's an API to use so we can avoid manual JSON parsing, so I thought you meant that we could use the internal test-only APIs and adding dependency was okay though non-ideal. I see what you mean now. I got rid of attributes_for_testing(). I still have RequestGlobalDump callback because MDM refuses to create dumps if it's not initialized with a RequestGlobalDumpFunction.
Patchset #7 (id:280001) has been deleted
On 2017/06/05 14:50:57, xunjieli wrote: > > My original suggestion above was to not depend on any of this (ReqGlobalDump, > > MemoryAllocatorDump, attributes_for_Testing, etc) and just use tracing > > end-to-end and then use TraceAnalyzer to read back the JSON. > > Quoting from #25: > > """ > > Is there any chance you can just turn trace (TraceLog::SetEnabled(), see > > MemoryDumpManagerTest) and look at the final trace result using > > base/test/trace_event_analyzer.h ? > > Those methods are not meant to be used outside of MDM and concretely we are > > going to refactor that stuff for the memory-uma v2 project so that would end > up > > making the refactoring more difficult. > > """ > > I think you just ended up increasing the dependencies on the internals here, > > which is a lose-lose in terms of code maintenance for both of us :) > > Sorry I just realized I might have explained myself poorly in the suggestion > > above. > > > > My original proposal was: use only TraceLog::SetEnabled/Disabled (like you do) > > and MDM::CreateProcessDump (it will bypass the multi-process logic that > requires > > all that boilerplate including the EXPECT_CALL stuff). Read back the trace > JSON > > using TraceAnalyzer. > > Advantage of this: you will not depend on any memory-infra internals > > (get_attributes_for_testing(), the TracedValue serialization, yada yada) but > > only on the final trace json, which is de facto a quite stable API surface > these > > days. > > Disadvantage: using TraceAnalyzer to get the data back might be a bit more > code > > than today's attributes_for_testing(). but at the same time you'll get rid of > > all the existing boilerplate for ReqGlobalDump > > > > This should allow you to avoid most of the boilerplate introduced here. > > > > If for any reason this shouldn't JustWork, I don't want to block this at this > > point I'd prefer going back to your original proposal (just invoke > OnMemoryDump > > on a mock MAD, without using tracelog nor MDM). > > Happy to IM if there is any doubt to get this unblocked > > Done. PTAL? > I misinterpreted your suggestion. Ned was asking if there's an API to use so we > can avoid manual JSON parsing, so I thought you meant that we could use the > internal test-only APIs and adding dependency was okay though non-ideal. I see > what you mean now. I got rid of attributes_for_testing(). I still have > RequestGlobalDump callback because MDM refuses to create dumps if it's not > initialized with a RequestGlobalDumpFunction. Thanks LGTM. I guess It didn't work because you didn't Initialize(). anyways don't worry. I am planning to do some cleanup and remove RequestGlobalDump from MDM at some point this week. as a result I will look into this and send a cl your way to figure out a way to fix it. My suggestion is that you land this first and I figure out how to sort out as part of my cleanup, instead of delaying your CL and asking you to sovle problems that I'm going to look into soon :)
On 2017/06/05 14:59:37, Primiano Tucci wrote: > On 2017/06/05 14:50:57, xunjieli wrote: > > > My original suggestion above was to not depend on any of this > (ReqGlobalDump, > > > MemoryAllocatorDump, attributes_for_Testing, etc) and just use tracing > > > end-to-end and then use TraceAnalyzer to read back the JSON. > > > Quoting from #25: > > > """ > > > Is there any chance you can just turn trace (TraceLog::SetEnabled(), see > > > MemoryDumpManagerTest) and look at the final trace result using > > > base/test/trace_event_analyzer.h ? > > > Those methods are not meant to be used outside of MDM and concretely we are > > > going to refactor that stuff for the memory-uma v2 project so that would end > > up > > > making the refactoring more difficult. > > > """ > > > I think you just ended up increasing the dependencies on the internals here, > > > which is a lose-lose in terms of code maintenance for both of us :) > > > Sorry I just realized I might have explained myself poorly in the suggestion > > > above. > > > > > > My original proposal was: use only TraceLog::SetEnabled/Disabled (like you > do) > > > and MDM::CreateProcessDump (it will bypass the multi-process logic that > > requires > > > all that boilerplate including the EXPECT_CALL stuff). Read back the trace > > JSON > > > using TraceAnalyzer. > > > Advantage of this: you will not depend on any memory-infra internals > > > (get_attributes_for_testing(), the TracedValue serialization, yada yada) but > > > only on the final trace json, which is de facto a quite stable API surface > > these > > > days. > > > Disadvantage: using TraceAnalyzer to get the data back might be a bit more > > code > > > than today's attributes_for_testing(). but at the same time you'll get rid > of > > > all the existing boilerplate for ReqGlobalDump > > > > > > This should allow you to avoid most of the boilerplate introduced here. > > > > > > If for any reason this shouldn't JustWork, I don't want to block this at > this > > > point I'd prefer going back to your original proposal (just invoke > > OnMemoryDump > > > on a mock MAD, without using tracelog nor MDM). > > > Happy to IM if there is any doubt to get this unblocked > > > > Done. PTAL? > > I misinterpreted your suggestion. Ned was asking if there's an API to use so > we > > can avoid manual JSON parsing, so I thought you meant that we could use the > > internal test-only APIs and adding dependency was okay though non-ideal. I see > > what you mean now. I got rid of attributes_for_testing(). I still have > > RequestGlobalDump callback because MDM refuses to create dumps if it's not > > initialized with a RequestGlobalDumpFunction. > > Thanks LGTM. I guess It didn't work because you didn't Initialize(). anyways > don't worry. I am planning to do some cleanup and remove RequestGlobalDump from > MDM at some point this week. as a result I will look into this and send a cl > your way to figure out a way to fix it. > My suggestion is that you land this first and I figure out how to sort out as > part of my cleanup, instead of delaying your CL and asking you to sovle problems > that I'm going to look into soon :) SG. Thanks!
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2875083002/#ps340001 (title: "Rebased")
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": 340001, "attempt_start_ts": 1496677470436650,
"parent_rev": "daca8cbeec27d5c816c22d948122ec22acaa3a26", "commit_rev":
"cc6b1d0925c81c56615211252251707a25214a6c"}
Message was sent while issue was closed.
Description was changed from ========== Add url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG=701387 ========== to ========== Add url_request_quic_perftest.cc This CL adds a simple perf test that checks the finish time of running 1000 requests against a HTTP/1.1 server that advertises QUIC support on an alternative host. This test also checks the end state of //net MemoryDumpProvider to make sure that there are no leftover HttpStreamFactoryImpl::Jobs or Quic Jobs. Follow-up CLs will add more complex tests cases. BUG=701387 Review-Url: https://codereview.chromium.org/2875083002 Cr-Commit-Position: refs/heads/master@{#477001} Committed: https://chromium.googlesource.com/chromium/src/+/cc6b1d0925c81c56615211252251... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:340001) as https://chromium.googlesource.com/chromium/src/+/cc6b1d0925c81c56615211252251... |
