{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/16353","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/16353/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/16353/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/16353/events","html_url":"https://github.com/dotnet/coreclr/pull/16353","id":296714613,"number":16353,"title":"Add more benchmarks to JitBench","user":{"login":"noahfalk","id":6243776,"avatar_url":"https://avatars0.githubusercontent.com/u/6243776?v=4","gravatar_id":"","url":"https://api.github.com/users/noahfalk","html_url":"https://github.com/noahfalk","followers_url":"https://api.github.com/users/noahfalk/followers","following_url":"https://api.github.com/users/noahfalk/following{/other_user}","gists_url":"https://api.github.com/users/noahfalk/gists{/gist_id}","starred_url":"https://api.github.com/users/noahfalk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/noahfalk/subscriptions","organizations_url":"https://api.github.com/users/noahfalk/orgs","repos_url":"https://api.github.com/users/noahfalk/repos","events_url":"https://api.github.com/users/noahfalk/events{/privacy}","received_events_url":"https://api.github.com/users/noahfalk/received_events","type":"User","site_admin":false},"labels":[{"id":390279535,"url":"https://api.github.com/repos/dotnet/coreclr/labels/*%20NO%20MERGE%20*","name":"* NO MERGE *","color":"e11d21","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":9,"created_at":"2018-02-13T12:08:26Z","updated_at":"2018-02-20T21:34:19Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/16353","html_url":"https://github.com/dotnet/coreclr/pull/16353","diff_url":"https://github.com/dotnet/coreclr/pull/16353.diff","patch_url":"https://github.com/dotnet/coreclr/pull/16353.patch"},"body":"So I’ve been working on this a little while and the scope snowballed from my initial plan, but wanted to let you know where things are now and get any high-level feedback you guys would like to provide. You are welcome to provide more detailed feedback if you want right now, but I imagine it will need at least one more pass before its ready to go and I'll ping again then.\r\n\r\nI had two main goals:\r\n1)\tAdd a few more scenario benchmarks – there are 3 new ones covering some csc and dotnet build test cases + the original music store scenario. I’m doing some work with Lee to identify other potential candidates but not trying to include that in this PR.\r\n2)\tMake the benchmarks easier to work with ad-hoc during development. The current automation works fine in CI, but if I want to create a coreclr build locally and compare a few different benchmarks across a few different configurations that takes a good number of commands + manual collation of the results from several log files (I always wind up referring back to Andy’s email because I can never remember all the different steps involved). I was also unable to build or run the JitBench project within VS and I guess I’m a sucker for being able to hit F5 and immediately debug the code I am writing ; )\r\n\r\nAt  the moment my ad-hoc usage looks about like this:\r\na)\tClone Coreclr repo\r\nb)\tbuild coreclr\r\nc)\tCd tests/src/performance/scenario/JitBench\r\nd)\tDotnet run -- --configs:Default,Tiering,Minopts --coreclr-bin-dir F:\\github\\coreclr\\bin\\Product\\Windows_NT.x64.Release\\\r\n\r\n```\r\n  === CONFIGURATION ===\r\n\r\nDotnetFrameworkVersion: 2.1.0-preview2-26131-06\r\nDotnetSdkVersion:       2.2.0-preview1-007558\r\nPrivateCoreCLRBinDir:   F:\\github\\coreclr\\bin\\Product\\Windows_NT.x64.Release\\\r\nArchitecture:           X64\r\nOutputDir:              C:\\Users\\noahfalk\\AppData\\Local\\Temp\\JitBench_2018_02_12_05_16_34_0611\r\nIterations:             10\r\nUseExistingSetup:       False\r\nConfigurations:         Default,Tiering,Minopts\r\n\r\n\r\nBenchmark run in progress...\r\nVerbose log: C:\\Users\\noahfalk\\AppData\\Local\\Temp\\JitBench_2018_02_12_05_16_34_0611\\JitBench_log.txt\r\n\r\n\r\n  === RESULTS ===\r\n\r\n       Benchmark                Metric           Default       Tiering      Minopts\r\n-----------------------  --------------------  ------------  -----------  ------------\r\nDotnet_Build_HelloWorld         Duration (ms)  1305.5+-0.98     1222+-15  1180.5+-0.98\r\n        Csc_Hello_World         Duration (ms)    618.5+-8.8      515+-12       496+-18\r\n      Csc_Roslyn_Source         Duration (ms)      2607+-12     2707+-45      2528+-28\r\n             MusicStore          Startup (ms)       724+-30  627.5+-0.98    610.5+-4.9\r\n             MusicStore    First Request (ms)       582+-21     528+-113   441.5+-0.98\r\n             MusicStore  Median Response (ms)         89+-0        89+-0         89+-0\r\n```\r\n\r\nThere are no required steps to build the test tree, restore anything, generate a layout, invoke xunit perf python scripts, set env vars or collate logs, though (bugs aside) it would still work roughly as before if you did all those things.\r\n\r\n\r\n\r\nHere are the major changes + a few outstanding questions:\r\n1)\tWould it cause issues to submit multiple benchmarks to benchview within a single csv? One concern I noticed is that we are collecting ETW PMC counters and encoding it in metrics such as “dotnet.exe!coreclr.dll, InstructionsRetired, ###”. It seems like counters named this way for multiple tests would inevitably collide if they were included in the same BenchView submission, but I'm not sure we use the data for any purpose right now?\r\n2)\tThe majority of code in JitBench wasn’t actually specific to MusicStore, rather it was handling things like command line parsing, process launching, logging, error checking, and building object models. After some refactoring the part of JitBench that is specific to MusicStore was factored out into about 250 lines (it used to be ~1000), the three new benchmarks added about ~50-75 lines each, and then the vast majority of the rest is general purpose. \r\n3)\tJitBench is now buildable via dotnet build (msbuild still works fine too), and the project type is now the standard dotnet SDK produced console app. On the good side this means you can run it in VS as-is, or just navigate to tests\\src\\performance\\scenario\\jitbench and use ‘dotnet run’. However that does make it different than most other test projects. When building with dotnet I had to disable using BuildTools on windows and instead just use the normal dotnet SDK targets, but the official build will continue to use msbuild/BuildTools. I also had to directly wire it up to the batch restore project list in the root of the test tree, otherwise I can’t properly restore it from both the full test build and a build of just the project directory. Last I had to tweak the python xunit runner to accept testBinLoc as either an exact file path or a directory. Dotnet projects produce a build output directory with multiple assemblies in it (the dependencies) and the python xunit runner would otherwise assume that every dll in that build output directory was a runnable benchmark. Explicitly telling it what assembly I wanted it to run resolves that issue.\r\nIt felt like I was cutting against the grain on this, but it also feels pretty useful to be able to use VS F5 and the standard dotnet SDK tools on our own projects. Let me know if you don’t like it though.\r\n4)\tI wound up doing some unnatural things with the XunitPerformanceHarness API mostly to avoid expanding the scope of the changes yet further into the xunit performance repo and perhaps because I was being overzealous about getting things to work a certain way, but let me know if you have other suggestions:\r\na.\tXunit performance expects to be given the raw command-line args so that it can parse them, but I find the choice to use ETW by default when no option has been specified aggravating here. The ETW events slow the results of these tests and I doubt the majority of our test scenarios make use of the data. Additionally it means the default case of launching from VS or running at the commandline will just give errors saying to relaunch with admin permissions. However making this not the default behavior meant I had to parse the commandline arguments myself in advance, modify the default case, and then reformat a new fake command-line. (Its actually not a lot of code, but its a weird interaction across components)\r\nb.\tThe tests do a bunch of setup at the beginning and if you request ETW + don’t give admin permissions I wanted to fail fast rather than fail when the runner eventually launches the first benchmark. Again this meant I have to pre-parse the command line and reason independently about whether XUnitPerformanceHarness will want to enable ETW.\r\nc.\tThe benchmarks need a working directory for various files they write and XunitPerformanceHarness expects specific command line formatting for how the output directory is passed. This was a third occasion in which I needed to understand and pre-parse the command line the harness wants in order to align the output directories.\r\nd.\tThe tests don’t use XUnitPerformance’s ScenarioBenchmark object model as their primary one. Instead I store the results in a similar OM that fit into the code a bit more smoothly and then spend ~100 lines of code transforming to the ScenarioBenchmark OM if/when I need to serialize the output. I am also not serializing the output at the same time I run the individual benchmarks. I create one XUnitPerformanceHarness instance each time I need to run a test, then optionally create a final one at the end just to serialize output.\r\ne.\tXunitPerformanceHarness has hard-coded console writeline usage inside it and I wanted all that output spew in a file to keep the console output clearer. I wound up hijacking Console.Out whenever using XunitPerformanceHarness in order to capture and redirect the logging.\r\n5)\tIn order to streamline using the tests, I wrote a small C# implementation of dotnet-install. The tests are still capable of using the private CoreCLR bits in CORE_ROOT or the result of runtest.cmd GenerateLayoutOnly, but that is now optional rather than required. \r\n6) The utilities folder I added has some general purpose file system manipulation, logging, and process running code that I grabbed from some of our internal sources as a convenience. In general the utilities are intended to be things that could be shared more widely in the future if we wanted to, they are not perf or benchmark specific.\r\n7) The runner folder, in addition to the shared portions of the old JitBench benchmark now has some very rudimentary test run functionality such as selecting a set of benchmarks to run, the configurations under which to run them, and logging configuration/progress/results.\r\n8) I flip-flopped and went back to calling CreateStore/Publish directly on MusicStore instead of using RunBenchmark. I needed to handle the dotnet installation for the other tests already and then it seemed bizarre if I couldn't get MusicStore to share the same one. Although we could refactor RunBenchmark.ps1 to skip dotnet setup too with a separate PR, the complexity to interface with RunBenchmark didn't seem worth the remaining functionality it would be providing at that point.  I've still preserved all the same stable versioning semantics as before so we shouldn't have lost any benefits on that front, just a shift of implementation.\r\n"},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/367127705","html_url":"https://github.com/dotnet/coreclr/pull/16353#issuecomment-367127705","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/16353","id":367127705,"user":{"login":"jorive","id":12551699,"avatar_url":"https://avatars0.githubusercontent.com/u/12551699?v=4","gravatar_id":"","url":"https://api.github.com/users/jorive","html_url":"https://github.com/jorive","followers_url":"https://api.github.com/users/jorive/followers","following_url":"https://api.github.com/users/jorive/following{/other_user}","gists_url":"https://api.github.com/users/jorive/gists{/gist_id}","starred_url":"https://api.github.com/users/jorive/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jorive/subscriptions","organizations_url":"https://api.github.com/users/jorive/orgs","repos_url":"https://api.github.com/users/jorive/repos","events_url":"https://api.github.com/users/jorive/events{/privacy}","received_events_url":"https://api.github.com/users/jorive/received_events","type":"User","site_admin":false},"created_at":"2018-02-20T21:34:19Z","updated_at":"2018-02-20T21:34:19Z","author_association":"MEMBER","body":"@AndyAyersMS Should I update the tools to those values?"}}
{"actor":{"display_login":"jorive"}}