Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1665303004:
Unflake CPU profiler tests. (Closed)
DescriptionUnflake CPU profiler tests.
Do not rely on elapsed time to collect enough samples.
Use CollectSample API function instead.
Remove checks for extra functions present in a profile, as
there in fact can be lots of native support functions.
BUG=v8:2999
LOG=N
Committed: https://crrev.com/1b6265eff71c73e64013299900520a01ec06baa8
Cr-Commit-Position: refs/heads/master@{#33822}
Patch Set 1 #
Total comments: 12
Patch Set 2 : drop v8_str #Patch Set 3 : revert back to stochastic sampling. #
Total comments: 14
Patch Set 4 : addressing yurys@ comments. #Patch Set 5 : Fix JsNative1JsNative2Js test #Patch Set 6 : make top level functions to be JS ones. #
Messages
Total messages: 37 (13 generated)
alph@chromium.org changed reviewers: + bmeurer@chromium.org, yurys@chromium.org
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665303004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665303004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... File test/cctest/profiler-extension.cc (right): https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... test/cctest/profiler-extension.cc:44: v8::Local<v8::String> v8_str(v8::Isolate* isolate, const char* string) { Please use the one defined in v8/test/cctest/cctest.h instead. https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... test/cctest/profiler-extension.cc:86: args.GetIsolate()->GetCpuProfiler()->CollectSample(); The whole idea of the tests that now call this native method was to test the sampler behavior with various stack states. When deepest js function simply calls native method, we will always have exit frame on top of the stack and no js frames above it. This is just one deterministic case among those we wanted to cover. Can we instead feed the sampler with an artificially constructed top frame (as we discussed a while ago) and probe into different pc positions inside js functions of interest? https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... test/cctest/test-cpu-profiler.cc:1099: TEST(FunctionCallSample) { This test doesn't check that Function.call gets into the profile, does it? https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... test/cctest/test-cpu-profiler.cc:1135: TEST(FunctionApplySample) { This test as its name implies should check that the profiler will sample Function.apply, otherwise it checks what is alredy covered by CollectCpuProfile.
https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... File test/cctest/profiler-extension.cc (right): https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... test/cctest/profiler-extension.cc:44: v8::Local<v8::String> v8_str(v8::Isolate* isolate, const char* string) { On 2016/02/05 17:33:04, yurys wrote: > Please use the one defined in v8/test/cctest/cctest.h instead. Done that, just forgot to upload. ;-) https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... test/cctest/profiler-extension.cc:86: args.GetIsolate()->GetCpuProfiler()->CollectSample(); On 2016/02/05 17:33:04, yurys wrote: > The whole idea of the tests that now call this native method was to test the > sampler behavior with various stack states. When deepest js function simply > calls native method, we will always have exit frame on top of the stack and no > js frames above it. This is just one deterministic case among those we wanted to > cover. Can we instead feed the sampler with an artificially constructed top > frame (as we discussed a while ago) and probe into different pc positions inside > js functions of interest? The tests I'm unflaking are mostly checking the stack collection for various combinations of stack frames (e.g. js-native-js, call, apply, deep stack, etc). And yes they can be done deterministically. The one you're suggesting is another test that I agree is worth adding. However it doesn't seem to belong to this patch, which intent is to enable important profiler tests currently disabled. wdyt? https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... test/cctest/test-cpu-profiler.cc:1099: TEST(FunctionCallSample) { On 2016/02/05 17:33:04, yurys wrote: > This test doesn't check that Function.call gets into the profile, does it? Does it have to be visible in the user callstack? I don't see a frame for it between start and bar. https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... test/cctest/test-cpu-profiler.cc:1135: TEST(FunctionApplySample) { On 2016/02/05 17:33:04, yurys wrote: > This test as its name implies should check that the profiler will sample > Function.apply, otherwise it checks what is alredy covered by CollectCpuProfile. It just checks that functions called through apply does also appear in the stack.
https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... File test/cctest/profiler-extension.cc (right): https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... test/cctest/profiler-extension.cc:86: args.GetIsolate()->GetCpuProfiler()->CollectSample(); On 2016/02/05 17:33:04, yurys wrote: > The whole idea of the tests that now call this native method was to test the > sampler behavior with various stack states. When deepest js function simply > calls native method, we will always have exit frame on top of the stack and no > js frames above it. This is just one deterministic case among those we wanted to > cover. Can we instead feed the sampler with an artificially constructed top > frame (as we discussed a while ago) and probe into different pc positions inside > js functions of interest? Ok, how about I drop the CheckChildrenNames which I believe is the main source of flakiness, while keep the stochastic sampling as is?
https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... File test/cctest/profiler-extension.cc (right): https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... test/cctest/profiler-extension.cc:86: args.GetIsolate()->GetCpuProfiler()->CollectSample(); On 2016/02/05 17:52:26, alph wrote: > The tests I'm unflaking are mostly checking the stack collection for various > combinations of stack frames (e.g. js-native-js, call, apply, deep stack, etc). > And yes they can be done deterministically. Well, the tests like JsNative1JsNative2JsSample are becoming ones always ending in a native call, i.e. JsNative1JsNative2JsSampleNative. In that case it can be simplified even further to be just JsNative1JsNative2, right? > The one you're suggesting is another test that I agree is worth adding. However > it doesn't seem to belong to this patch, which intent is to enable important > profiler tests currently disabled. > wdyt? I agree that it is important to find a way to fix and enable those test as having them skipped adds little value. https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... test/cctest/test-cpu-profiler.cc:1099: TEST(FunctionCallSample) { On 2016/02/05 17:52:26, alph wrote: > On 2016/02/05 17:33:04, yurys wrote: > > This test doesn't check that Function.call gets into the profile, does it? > > Does it have to be visible in the user callstack? I don't see a frame for it > between start and bar. According to the table above it should be visible if we took a sample inside call stub. Do we care about that case? IIRC, when the control flow hits the callee we cannot tell if it was regular function invocation or a call by means of call/apply. https://codereview.chromium.org/1665303004/diff/1/test/cctest/test-cpu-profil... test/cctest/test-cpu-profiler.cc:1135: TEST(FunctionApplySample) { On 2016/02/05 17:52:26, alph wrote: > On 2016/02/05 17:33:04, yurys wrote: > > This test as its name implies should check that the profiler will sample > > Function.apply, otherwise it checks what is alredy covered by > CollectCpuProfile. > > It just checks that functions called through apply does also appear in the > stack. See my reply above.
On 2016/02/05 18:03:33, alph wrote: > https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... > File test/cctest/profiler-extension.cc (right): > > https://codereview.chromium.org/1665303004/diff/1/test/cctest/profiler-extens... > test/cctest/profiler-extension.cc:86: > args.GetIsolate()->GetCpuProfiler()->CollectSample(); > On 2016/02/05 17:33:04, yurys wrote: > > The whole idea of the tests that now call this native method was to test the > > sampler behavior with various stack states. When deepest js function simply > > calls native method, we will always have exit frame on top of the stack and no > > js frames above it. This is just one deterministic case among those we wanted > to > > cover. Can we instead feed the sampler with an artificially constructed top > > frame (as we discussed a while ago) and probe into different pc positions > inside > > js functions of interest? > > Ok, how about I drop the CheckChildrenNames which I believe is the main source > of flakiness, while keep the stochastic sampling as is? It sounds like a better approach to me if that's going to be enough.
ptal
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665303004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665303004/40001
https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:583: " try {\n" Can you also replace try/catch with %NeverOptimizeFunction here and in the other places you are changing? https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1128: reinterpret_cast<i::CpuProfile*>(profile)->Print(); RunProfiler already does this. Please remove. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1180: reinterpret_cast<i::CpuProfile*>(profile)->Print(); Ditto. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1183: const v8::CpuProfileNode* startNode = GetChild(env.local(), root, "start"); style nit: here and in other places start_node instead of startNode https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1381: .ToLocalChecked(); Place on the previous line? https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1395: v8::CpuProfile* profile = RunProfiler(env, function, NULL, 0, 1000); Why is it not enough to have a single sample here? https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1483: reinterpret_cast<i::CpuProfile*>(profile)->Print(); Please remove.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665303004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665303004/60001
ptal https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... File test/cctest/test-cpu-profiler.cc (right): https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:583: " try {\n" On 2016/02/05 22:54:11, yurys wrote: > Can you also replace try/catch with %NeverOptimizeFunction here and in the other > places you are changing? Done. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1128: reinterpret_cast<i::CpuProfile*>(profile)->Print(); On 2016/02/05 22:54:11, yurys wrote: > RunProfiler already does this. Please remove. Done. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1180: reinterpret_cast<i::CpuProfile*>(profile)->Print(); On 2016/02/05 22:54:11, yurys wrote: > Ditto. Done. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1183: const v8::CpuProfileNode* startNode = GetChild(env.local(), root, "start"); On 2016/02/05 22:54:11, yurys wrote: > style nit: here and in other places start_node instead of startNode Done. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1381: .ToLocalChecked(); On 2016/02/05 22:54:11, yurys wrote: > Place on the previous line? I can't. It's git cl format. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1395: v8::CpuProfile* profile = RunProfiler(env, function, NULL, 0, 1000); On 2016/02/05 22:54:11, yurys wrote: > Why is it not enough to have a single sample here? Because as you said it won't be a JS sample on top. https://codereview.chromium.org/1665303004/diff/40001/test/cctest/test-cpu-pr... test/cctest/test-cpu-profiler.cc:1483: reinterpret_cast<i::CpuProfile*>(profile)->Print(); On 2016/02/05 22:54:11, yurys wrote: > Please remove. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/1034) v8_linux_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665303004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665303004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665303004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665303004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm.
LGTM.
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665303004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665303004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Unflake CPU profiler tests. Do not rely on elapsed time to collect enough samples. Use CollectSample API function instead. Remove checks for extra functions present in a profile, as there in fact can be lots of native support functions. BUG=v8:2999 LOG=N ========== to ========== Unflake CPU profiler tests. Do not rely on elapsed time to collect enough samples. Use CollectSample API function instead. Remove checks for extra functions present in a profile, as there in fact can be lots of native support functions. BUG=v8:2999 LOG=N Committed: https://crrev.com/1b6265eff71c73e64013299900520a01ec06baa8 Cr-Commit-Position: refs/heads/master@{#33822} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1b6265eff71c73e64013299900520a01ec06baa8 Cr-Commit-Position: refs/heads/master@{#33822} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
