tools: fix flakiness in test-tick-processor #2694
Conversation
| @@ -18,6 +18,6 @@ if [ ! -x "$NODE" ] && [ -x "$(dirname "$0")/../../node" ]; then | |||
| NODE="$(dirname "$0")/../../node" | |||
| fi | |||
|
|
|||
| "$NODE" "$TEMP_SCRIPT_FILE" $@ | |||
| "$NODE" "$TEMP_SCRIPT_FILE" "--call-graph-size=10" $@ | |||
There was a problem hiding this comment.
Can I suggest passing this as an argument in the test?
|
The second commit needs some rewording to conform to the guidelines, e.g. |
|
Sorry about that, fixed. |
|
LGTM |
|
Works for me. I'd still prefer something deterministic ( |
|
@Trott It is definitely possible but would require tuning that number of iterations for the different processor speeds to make sure we have sufficient time to get enough ticks. Writing OS specific code seems fine but I hesitate to write code that is dependent on processor speed. If this is something that is important I can investigate it further. |
|
@Trott Since we are doing time-based sampling, you want the test to run at least for ~2 seconds on a fast machine so that we get a reasonable sample. At the same time you don't want the test to run for 'a very long time' on slow machines such as a heavily loaded RPi. IMO, the current solution of killing the test after 2 seconds is pretty reasonable as it runs for at least 2 seconds on the fastest machine of the future, while still working on the slowest machine from today. |
|
@matthewloring As part of the "fix flakiness" commit, you can remove the test from the list of flaky tests. Or add a separate commit, as you prefer. Just remove this line: https://github.com/matthewloring/io.js/blob/bcc1bb71bae06ef8ec746d6800416c0c53476aa7/test/parallel/parallel.status#L15 |
|
Unfortunately, I just discovered a new way that this test can flake. It is less frequent (this PR still improves things) but I don't think it should be removed from the list of flaky tests just yet. I need to dig in further and try to figure out whether this is something that can be fixed in Node or whether I have to continue this in V8. |
|
I now have separate code snippets to test for JS and C++ symbols so that they can be tailored to have a higher percentage of the desired tick type. The remaining flakiness in the JS symbol check seemed to be caused by missed code-creation events in the raw log output. To correct for this, I see if the desired JS symbols are present or a percentage of UNKNOWN ticks which are registered if the code creation event is missed. This event is missed roughly 1/600 iterations after testing on my local machine. This is still considered a success since the scripts inside node are processing the log as desired. These missed code creation events need to be further investigated inside v8. The new test passed 1000/1000 times so hopefully the flakiness is gone for good now. Could we run a CI to make sure the changes to the test don't break on other platforms (tested on Mac and Ubuntu locally). @joaocgreis I have removed this test from the list of flaky ones. |
|
|
|
Thanks for the CI! The only failure, test-child-process-emfile.js on freebsd101-64, appears unrelated. @ofrobots @bnoordhuis Could you please take another look. |
| ' setImmediate(function() { f(); });' + | ||
| '};' + | ||
| 'setTimeout(function() { process.exit(0); }, 2000);' + | ||
| 'f();'); |
There was a problem hiding this comment.
Suggestion: use template strings for legibility here and below.
|
LGTM with a nit and a suggestion. |
Per the discussion on #2471, the JS symbols checked for by this test were occasionally too deep in the stack and were being ignored by the tick processor. I have addressed this by increasing the stack depth inspected by the tick processor and looking for the eval symbol which is more likely to be present. Additional flakiness was caused by occasional misses of the code creation event for the JS function being executed. I now have separate code snippets to test for JS and C++ symbols and if the code creation event is missed for the JS symbol test then I check for a percentage of UNKNOWN symbols in processed output. This is considered a success as the processing scripts in the node repository are still correctly processing the ticks recieved from the v8 scripts. Further investigation is needed into the v8 profiling scripts to determine why code creation events are being missed.
The polyfill is only needed if incorrect command line arguments are passed to the script so it was missed in initial testing.
Per the discussion on #2471, the JS symbols checked for by this test were occasionally too deep in the stack and were being ignored by the tick processor. I have addressed this by increasing the stack depth inspected by the tick processor and looking for the eval symbol which is more likely to be present. Additional flakiness was caused by occasional misses of the code creation event for the JS function being executed. I now have separate code snippets to test for JS and C++ symbols and if the code creation event is missed for the JS symbol test then I check for a percentage of UNKNOWN symbols in processed output. This is considered a success as the processing scripts in the node repository are still correctly processing the ticks recieved from the v8 scripts. Further investigation is needed into the v8 profiling scripts to determine why code creation events are being missed. PR-URL: #2694 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The polyfill is only needed if incorrect command line arguments are passed to the script so it was missed in initial testing. PR-URL: #2694 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Thanks Matt, landed in a85f4b5...40ec84d. |
Per the discussion on #2471, the JS symbols checked for by this test were occasionally too deep in the stack and were being ignored by the tick processor. I have addressed this by increasing the stack depth inspected by the tick processor and looking for the eval symbol which is more likely to be present. Additional flakiness was caused by occasional misses of the code creation event for the JS function being executed. I now have separate code snippets to test for JS and C++ symbols and if the code creation event is missed for the JS symbol test then I check for a percentage of UNKNOWN symbols in processed output. This is considered a success as the processing scripts in the node repository are still correctly processing the ticks recieved from the v8 scripts. Further investigation is needed into the v8 profiling scripts to determine why code creation events are being missed. PR-URL: #2694 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The polyfill is only needed if incorrect command line arguments are passed to the script so it was missed in initial testing. PR-URL: #2694 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Per the discussion on #2471, the JS symbols checked for by this test were occasionally too deep in the stack and were being ignored by the tick processor. I have addressed this by increasing the stack depth inspected by the tick processor and looking for the eval symbol which is more likely to be present. Additional flakiness was caused by occasional misses of the code creation event for the JS function being executed. I now have separate code snippets to test for JS and C++ symbols and if the code creation event is missed for the JS symbol test then I check for a percentage of UNKNOWN symbols in processed output. This is considered a success as the processing scripts in the node repository are still correctly processing the ticks recieved from the v8 scripts. Further investigation is needed into the v8 profiling scripts to determine why code creation events are being missed. PR-URL: #2694 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The polyfill is only needed if incorrect command line arguments are passed to the script so it was missed in initial testing. PR-URL: #2694 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Per the discussion on #2471, the JS symbols checked for by this test
were occasionally too deep in the stack and were being ignored by the
tick processor.
I have addressed this by increasing the stack depth inspected by the
tick processor and looking for the eval symbol which is more likely
to be present.
I also sneaked in a polyfill that was missing from the original PR.
It is only a problem if incorrect command line arguments are passed
to the script so it was missed in initial testing.
/cc @ofrobots @bnoordhuis @Fishrock123 @joaocgreis @Trott