Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: fix flakiness in test-tick-processor #2694

Closed
wants to merge 2 commits into from
Closed

tools: fix flakiness in test-tick-processor #2694

wants to merge 2 commits into from

Conversation

matthewloring
Copy link

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

@mscdex mscdex added the tools Issues and PRs related to the tools directory. label Sep 4, 2015
@Fishrock123
Copy link
Contributor

@@ -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" $@
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I suggest passing this as an argument in the test?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely! Done.

@bnoordhuis
Copy link
Member

The second commit needs some rewording to conform to the guidelines, e.g. tools: add missing tick processor polyfill.

@matthewloring
Copy link
Author

Sorry about that, fixed.

@bnoordhuis
Copy link
Member

LGTM

@Trott
Copy link
Member

Trott commented Sep 4, 2015

Works for me. I'd still prefer something deterministic (execute the function recursively 1200 times, then check the stack over the current run for two seconds and then check the stack) but there may be nuances to the test that I'm missing that preclude that approach.

@matthewloring
Copy link
Author

@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.

@ofrobots
Copy link
Contributor

ofrobots commented Sep 4, 2015

@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.

@joaocgreis
Copy link
Member

@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

@matthewloring
Copy link
Author

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.

@matthewloring
Copy link
Author

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.

@evanlucas
Copy link
Contributor

@Trott
Copy link
Member

Trott commented Sep 14, 2015

YESSSSS!

@matthewloring
Copy link
Author

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();');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: use template strings for legibility here and below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@bnoordhuis
Copy link
Member

LGTM with a nit and a suggestion.

Matt Loring added 2 commits September 14, 2015 14:56
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.
bnoordhuis pushed a commit that referenced this pull request Sep 14, 2015
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>
bnoordhuis pushed a commit that referenced this pull request Sep 14, 2015
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>
@bnoordhuis
Copy link
Member

Thanks Matt, landed in a85f4b5...40ec84d.

@bnoordhuis bnoordhuis closed this Sep 14, 2015
Fishrock123 pushed a commit that referenced this pull request Sep 15, 2015
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>
Fishrock123 pushed a commit that referenced this pull request Sep 15, 2015
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>
@Fishrock123 Fishrock123 mentioned this pull request Sep 15, 2015
7 tasks
rvagg pushed a commit that referenced this pull request Sep 15, 2015
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>
rvagg pushed a commit that referenced this pull request Sep 15, 2015
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>
@rvagg rvagg mentioned this pull request Sep 15, 2015
@rvagg rvagg mentioned this pull request Sep 22, 2015
@matthewloring matthewloring deleted the tick-flaky-fix branch February 25, 2016 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants