Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

jorive
Copy link

@jorive jorive commented May 25, 2017

  • Update the xUnit Performance package with a bug fixed that was silently making the tests fail
  • Removed the [MeasureInstructionsRetired] attribute from the benchmarks source code, as it's not use, the reason the tests were failing, and this is now passed on the command line when profiling the tests.
  • Updated the run-xunit-perf.sh and perf.groovy files with the appropriate changes.

echo "----------"
echo " Running $testname"
echo "----------"
run_command $stabilityPrefix ./corerun PerfHarness.dll $test --perf:runid Perf --perf:collect stopwatch || exit 1
Copy link

@dpodder dpodder May 25, 2017

Choose a reason for hiding this comment

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

This hardcodes --perf:collect and stopwatch, ignoring the input parameters passed to this script. Is that intentional? #Closed

Copy link

@dpodder dpodder left a comment

Choose a reason for hiding this comment

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

LGTM

@AndyAyersMS
Copy link
Member

It feels wrong to me to remove the [MeasureInstructionsRetired] annotations -- wouldn't it have been better to fix xunit-perf to handle the exception that this causes on Linux and / or fix whatever the underlying issue is that causes the exception?

However, I'm not going to hold up the PR over this, since I'd like to get the Linux measurements unblocked.

Are you able to trigger perf runs pre-merge and verify they produce the expected data, or are we held hostage to lagging visibility of changes to perf.groovy?

@jorive
Copy link
Author

jorive commented May 25, 2017

@AndyAyersMS [MeasureInstructionsRetired] is fixed in the xUnit version that was also updated in this PR. In addition, this is still measure in the profile=on runs, and disabled on profile=off, so you still get the Instructions Retired results through PR and rolling runs.

@jorive
Copy link
Author

jorive commented May 25, 2017

@AndyAyersMS I was able to run the benchmarks offline, and I am in the process to add a new machine to Jenkins (Old machine hardware died), so I can tests the E2E scenario and validate.

@DrewScoggins
Copy link
Member

test ci please

@jorive
Copy link
Author

jorive commented May 25, 2017

@dotnet-bot test Ubuntu14.04 perf
@dotnet-bot test Windows_NT x64 perf
@dotnet-bot test Windows_NT x86 perf

@DrewScoggins
Copy link
Member

test ci please

2 similar comments
@DrewScoggins
Copy link
Member

test ci please

@jorive
Copy link
Author

jorive commented May 26, 2017

test ci please

@jorive
Copy link
Author

jorive commented May 26, 2017

@DrewScoggins I just set the maximum number of iterations, on the perf.groovy, to match the Windows runs too.

@jorive jorive merged commit 70ea7d4 into dotnet:master May 27, 2017
@jorive jorive deleted the dev/Enable-Linux-Perf-Runs branch May 27, 2017 01:01
brianrob pushed a commit to brianrob/coreclr that referenced this pull request Jun 6, 2017
- Removing `[assembly: MeasureInstructionsRetired]` from performance tests.
- Due to this issue: microsoft/xunit-performance#231, we are blocked from running on performance tests on Linux.
  The tag is not used by infrastructure and it is passed during runtime when profiling on Windows.
- Adding the option to execute CoreRun with the stability prefix.
- Moving file to be archived by Jenkins and updating groovy file.
- Setting the maximum number of iterations.
brianrob added a commit that referenced this pull request Jun 6, 2017
* Enable linux perf runs (#11905)

- Removing `[assembly: MeasureInstructionsRetired]` from performance tests.
- Due to this issue: microsoft/xunit-performance#231, we are blocked from running on performance tests on Linux.
  The tag is not used by infrastructure and it is passed during runtime when profiling on Windows.
- Adding the option to execute CoreRun with the stability prefix.
- Moving file to be archived by Jenkins and updating groovy file.
- Setting the maximum number of iterations.

* Updating version of xUnit-Performance-Api (#11987)

- This update contains a fix for this issue: microsoft/xunit-performance#230
- Consolidate the Api version in two places: `dependencies.prop` and `PerfHarness.csproj`
- Move performance log files to root directory where Jenkins can archive them
- Fixed the output to console, so it is easier to see on the Jenkins job the sccript steps live and get an exact repro step if needed
@karelz karelz modified the milestone: 2.1.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants