Dick bits #2965
Dick bits #2965
Conversation
2. set Optimise=true (Release build) as Benchmark guidelines 3. append Readkey so results viewable 4. make [some] methods static as analyzer suggestions 5. add .editorconfig file for contributor consistency
|
|
There was a problem hiding this comment.
@DickBaker while some of your changes are interesting, the intent behind this specific benchmark is to show users how to set up a quick benchmark with minimal complexity, rather than to actually produce the most accurate results and write the most powerful/flexible/comprehensive benchmark. Most of your changes seem like they would make things harder for user new to benchmarking and EF.
It's best to understand exactly what it is you're trying to achieve here, keeping in mind the minimalistic goal for this benchmark.
Some specific comments:
The calculations sum / count as-is yield int when should yield double
This is technically correct, though it wouldn't have any bearing on what this benchmark is actually doing. We can change this though.
Need optimise=true in the .csproj to satisfy Benchmark.net
This is a good idea, as it would allow users to get up and running more quickly.
needs a Console.Read() at end so results can be viewed/kept
This depends on how you're running the benchmarks - I'm assuming you're on VS, where the terminal window gets closed immediately; when running from the commandline, or from an IDE such as Jetbrains Rider, the additional ReadKey is actually disruptive. IMO this doesn't really belong in the benchmark code.
.editorconfig
Outdated
| @@ -0,0 +1,200 @@ | |||
| # editorconfig.org | |||
There was a problem hiding this comment.
Having a .editorconfig for the samples isn't necessary a bad idea, but it's a good idea to split this off to another PR. Specifically, there are two different .editorconfig files in this PR, which also aren't the same as the one we currently use in EF Core itself.
There was a problem hiding this comment.
I noticed that my CodeCleanup had wobbled some of the formatting, so I added mine as an explicit .editorconfig rather than have ambiguity. Got carried away and added 2 [whoops, like NYC so good they named it twice]. Happy that you should ditch my templates in favour of an "official one". I liked the simplicity of Roji's original that seemed to command instant wisdom of just pulling what you actually need (just cols/rows critters required not the whole prairie], but spotting the integer-division just had to follow-thru with a full-on OCD response! Honestly, not trying to over-complicate Roji's simple v1 main-dish, but to offer some vn+1 dessert as next course [maybe this should appear later once the newbie has joined the party and not frightened by too much noise. Yeah, maybe.
Anyway I have been exploring various Find/First style as well as Cold/Hot caching and even few basic ADO flavours for comparison & horses/courses. Missing so far is a commentary of when/what suitable, but I hope the BM results will speak for themselves. Your call, and I hope to revise PR 2965 with much more meat shortly and hope doesn't give you too much indigestion as pure vegans. [ok I give up speech-writing and will stay with my day-job!].
Thanks for your launching this perf journey as I find on my consultancy journey that there are many "unknown unknowns" that need a strong torch.
|
@DickBaker thanks for understanding and explaining - we're very interested in any perf results you find during your investigations. Specifically for this PR and the samples... If you'd like to amend this PR to only contain the more minor improvements I mentioned above (e.g. add the division, the optimize-true), we can definitely merge that. Let me know how you'd like to proceed. |
FYI I left in DIYMark as debug convenience [drop before merge] PK driven like most real-world scenarios [AK too wide etc] request/sequential/random and caching [avoid r-trip] cases 20 methods = 3xRoji, 5xADO, 4xFind, 2xFirst,2xHybrid, 2xObj, 2xDict perhaps investigate BM RunStrategy, MaxIterationCount, SimpleJob, ShortRunJob
|
Am closing this as the relevant changes have been committed via #2990 |
calculate Rating as Double, optimise=true and other tweaks
Issue #2964