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

Change input file finding mechanism for BenchmarksGame tests#9281

Closed
wtgodbe wants to merge 1 commit into
dotnet:masterfrom
wtgodbe:BenchMarksGames
Closed

Change input file finding mechanism for BenchmarksGame tests#9281
wtgodbe wants to merge 1 commit into
dotnet:masterfrom
wtgodbe:BenchMarksGames

Conversation

@wtgodbe
Copy link
Copy Markdown
Member

@wtgodbe wtgodbe commented Feb 2, 2017

These tests were looking for their input file relative to CORE_ROOT - in Helix, the directory structure assumed by these tests is not maintained. Instead, CORE_ROOT points to "some directory somewhere" which contains the runtime dependencies, while the tests live somewhere else entirely. This PR changes the mechanism for finding input files in 2 JIT tests, such that we always looks for them in the same directory as the executing assembly, instead of looking for them relative to the location of Core_Root.

@wtgodbe
Copy link
Copy Markdown
Member Author

wtgodbe commented Feb 2, 2017

@MattGal @gkhanna79 @AndyAyersMS PTAL

@AndyAyersMS
Copy link
Copy Markdown
Member

Looks good to me, in fact I was trying to figure out how to do that but couldn't. Can you verify that running these tests via tests\scripts\run-xunit-perf still works too?

@wtgodbe
Copy link
Copy Markdown
Member Author

wtgodbe commented Feb 2, 2017

@AndyAyersMS working on that now.

@wtgodbe wtgodbe force-pushed the BenchMarksGames branch 4 times, most recently from b8276db to b005684 Compare February 2, 2017 22:50
string inputPathPerf = Path.Combine(pathPartsPerf);
// Input file will end up next to the assembly
string currentDirectory = AppContext.BaseDirectory;
string[] inputPathParts = new string[] {currentDirectory, inputFile};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like you don't need to put currentDirectory into a variable, could just use it in the array initializer?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah you're right, I actually had it that way initially, but then changed to this to see if the error was caused by some weird behavior in C# array initializers - I want to see if the Pri-1 test job passes in CI, then I'll make that change after

@MattGal
Copy link
Copy Markdown
Member

MattGal commented Feb 2, 2017

Lgtm.

@gkhanna79
Copy link
Copy Markdown
Member

CC @RussKeldorph @pgavlin

@wtgodbe
Copy link
Copy Markdown
Member Author

wtgodbe commented Feb 3, 2017

The plot thickens - AppContext.BaseDirectory does indeed give you the directory of the currently executing assembly - but in this case, that's not the JIT tests in question, but rather CoreRun.exe. So the tests are looking for their input files in Core_Root, instead of in their own directory. I'm not sure there's a way to get the directory of the executable being run by CoreRun, from inside of that executable - any ideas @AndyAyersMS @gkhanna79 @MattGal? Otherwise we'll need some other solution, like including the contents of the text file in the .cs file itself (like https://github.com/dotnet/coreclr/blob/master/tests/src/JIT/jit64/opt/cse/HugeArray.cs does)

@AndyAyersMS
Copy link
Copy Markdown
Member

I tried getting the assembly location from a type in the test assembly but could not get it to work, though it seemed like it should. Also the big input files are 25MB (I held off on using the "real" big input files which are 250MB) so embedding them could be painful.

@gkhanna79
Copy link
Copy Markdown
Member

@wtgodbe you need to use an approach like https://github.com/dotnet/corefx/blob/master/src/System.Reflection/tests/Common.cs#L107 - that is, get assembly corresponding to a type in the test assembly.

@wtgodbe wtgodbe force-pushed the BenchMarksGames branch 2 times, most recently from b7af479 to a0d1bd6 Compare February 3, 2017 18:43
@wtgodbe
Copy link
Copy Markdown
Member Author

wtgodbe commented Feb 3, 2017

I just spoke with @DrewScoggins, and the reason that Assembly.Location isn't available here is that JIT performance tests are locked into using NetStandard1.4 (https://github.com/dotnet/coreclr/blob/master/tests/src/JIT/config/benchmark/project.json#L34), which doesn't have Assembly.Location. Once the performance tests move from using a Desktop runner to a core-based runner, we'll be able to move everything to netcoreapp, and at that point this problem will be solveable. Drew told me that will happen this month, so in the interim I'm going to disable these tests, & file an issue against the both of us to re-enable them once the perf tests move away from Desktop.

@wtgodbe wtgodbe closed this Feb 3, 2017
@gkhanna79
Copy link
Copy Markdown
Member

Very well - please have an issue filed to renable the tests and have it assigned to @DrewScoggins or @AndyAyersMS for tracking.

Likewise, please disable CoreMangLib tests (globalization files) and reflect it in an issue against Tarek.

@AndyAyersMS
Copy link
Copy Markdown
Member

Can you just disable for helix somehow?

These work fine as-is both in CI and in the xunit perf testing the Jit team does, and are tests we'd very much like to track over time.

@wtgodbe
Copy link
Copy Markdown
Member Author

wtgodbe commented Feb 3, 2017

@AndyAyersMS yes, I'm only disabling them for Helix - #9315

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