Skip to content

PerfScore initial support #215

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.

Sign up for GitHub

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

Jump to bottom
Closed
briansull wants to merge 3 commits into from
Closed

PerfScore initial support #215

briansull wants to merge 3 commits into from

Conversation

@briansull
Copy link
Contributor

@briansull briansull commented Oct 4, 2019

Added support for --perfscore to jit-diff.cs
Added support for --count pass through to jit-analyze
Changed default for --count from 5 to 20

@briansull
Perf Score initial change
d3106fc 
Added support for --perfscore to jit-diff.cs
Added support for --count pass through to jit-analyze
Changed default for --count from 5 to 20
@briansull
Copy link
Contributor Author

briansull commented Oct 4, 2019

Sample output:

d:\fxkit\coreclr>jit-diff diff --pmi --base D:\fxkit\baseline\coreclr\bin\Product\Windows_NT.x64.Checked --diff D:\fxkit\coreclr\bin\Product\Windows_NT.x64.Checked --core_root D:\fxkit\coreclr\bin\tests\Windows_NT.x64.Release\Tests\Core_Root --benchmarks --tsv --perfscore --count 20
Using --output d:\fxkit\coreclr\bin\diffs
Using --arch x64
Using --test_root d:\fxkit\coreclr\bin\tests\Windows_NT.x64.Release
Beginning PMI Diffs for benchstones and benchmarks game in d:\fxkit\coreclr\bin\tests\Windows_NT.x64.Release
/ Finished 82/82 Base 82/82 Diff [105.1 sec]
Completed PMI Diffs for benchstones and benchmarks game in d:\fxkit\coreclr\bin\tests\Windows_NT.x64.Release in 105.43s
Diffs (if any) can be viewed by comparing: d:\fxkit\coreclr\bin\diffs\dasmset_45\base d:\fxkit\coreclr\bin\diffs\dasmset_45\diff
Analyzing diffs...
Found 23 files with textual diffs.
PMI Diffs for benchstones and benchmarks game in d:\fxkit\coreclr\bin\tests\Windows_NT.x64.Release for x64 default jit
Summary:
(Lower is better)
Total perfScore of diff: 1,648.25 (0.24% of base)
    diff is a regression.
Top file regressions by perfScore:
          448.00 : Benchstones\BenchF\FFT\FFT\FFT.dasm (3.06% of base)
          268.20 : Benchstones\BenchF\MatInv4\MatInv4\MatInv4.dasm (2.04% of base)
          234.30 : Bytemark\Bytemark\Bytemark.dasm (0.16% of base)
          156.30 : Benchstones\BenchF\SqMtx\SqMtx\SqMtx.dasm (6.04% of base)
          103.05 : V8\Crypto\Crypto\Crypto.dasm (0.52% of base)
          102.30 : Burgers\Burgers\Burgers.dasm (1.20% of base)
           97.30 : Benchstones\BenchF\LLoops\LLoops\LLoops.dasm (0.30% of base)
           78.10 : BenchmarksGame\fannkuch-redux\fannkuch-redux-5\fannkuch-redux-5.dasm (1.93% of base)
           64.00 : Benchstones\BenchI\AddArray2\AddArray2\AddArray2.dasm (2.07% of base)
           36.10 : BenchmarksGame\fasta\fasta-1\fasta-1.dasm (0.69% of base)
           35.10 : BenchmarksGame\n-body\n-body-3\n-body-3.dasm (1.10% of base)
           14.00 : SciMark\SciMark\SciMark.dasm (0.04% of base)
            9.20 : BenchmarksGame\spectralnorm\spectralnorm-3\spectralnorm-3.dasm (0.40% of base)
            1.50 : Benchstones\BenchI\NDhrystone\NDhrystone\NDhrystone.dasm (0.09% of base)
            1.30 : Benchstones\BenchF\InvMt\InvMt\InvMt.dasm (0.04% of base)
            0.60 : BenchmarksGame\k-nucleotide\k-nucleotide-1\k-nucleotide-1.dasm (0.04% of base)
Top file improvements by perfScore:
           -0.60 : Benchstones\BenchI\Array1\Array1\Array1.dasm (-0.05% of base)
           -0.20 : Serialization\Deserialize\Deserialize.dasm (-0.01% of base)
           -0.20 : Serialization\Serialize\Serialize.dasm (-0.01% of base)
           -0.10 : Benchstones\BenchI\BubbleSort\BubbleSort\BubbleSort.dasm (-0.01% of base)
20 total files with score differences (4 improved, 16 regressed), 62 unchanged.
Top method regressions by total perfscore change:
      448.00 ( 3.23% of base) : Benchstones\BenchF\FFT\FFT\FFT.dasm - FFT:FastFourierT(ref,ref,int)
      179.20 ( 7.93% of base) : Benchstones\BenchF\MatInv4\MatInv4\MatInv4.dasm - MatInv4:MProd(ref,ref,ref,byref)
      156.30 ( 9.24% of base) : Benchstones\BenchF\SqMtx\SqMtx\SqMtx.dasm - SqMtx:Inner(ref,ref,int)
       97.30 ( 0.43% of base) : Benchstones\BenchF\LLoops\LLoops\LLoops.dasm - LLoops:Main1(int):this
       64.00 ( 5.76% of base) : Benchstones\BenchI\AddArray2\AddArray2\AddArray2.dasm - AddArray2:BenchInner1(ref,byref)
       53.60 ( 4.29% of base) : Burgers\Burgers\Burgers.dasm - Burgers:GetCalculated3(int,int,double,double,double,ref):ref
       48.90 ( 3.80% of base) : BenchmarksGame\n-body\n-body-3\n-body-3.dasm - NBodySystem:Energy():double:this
       45.50 ( 4.64% of base) : BenchmarksGame\fannkuch-redux\fannkuch-redux-5\fannkuch-redux-5.dasm - FannkuchRedux_5:firstPermutation(ref,ref,ref,int)
       44.50 ( 1.00% of base) : Benchstones\BenchF\MatInv4\MatInv4\MatInv4.dasm - MatInv4:MinV1(ref,byref,byref,ref,ref)
       41.00 ( 0.50% of base) : Bytemark\Bytemark\Bytemark.dasm - AssignJagged:second_assignments(ref,ref)
       37.70 ( 0.72% of base) : Benchstones\BenchF\MatInv4\MatInv4\MatInv4.dasm - MatInv4:MinV2(ref,byref,byref,ref,ref)
       36.10 ( 4.47% of base) : BenchmarksGame\fasta\fasta-1\fasta-1.dasm - Fasta_1:SelectNucleotides(ref,ref):ref
       32.60 ( 1.70% of base) : BenchmarksGame\fannkuch-redux\fannkuch-redux-5\fannkuch-redux-5.dasm - FannkuchRedux_5:run(int,int,int)
       28.00 ( 0.39% of base) : Bytemark\Bytemark\Bytemark.dasm - Huffman:DoHuffIteration(ref,ref,ref,int,int,ref):long
       26.80 ( 3.27% of base) : Bytemark\Bytemark\Bytemark.dasm - Neural:do_mid_forward(int)
       25.30 ( 1.74% of base) : Burgers\Burgers\Burgers.dasm - Burgers:GetCalculated1(int,int,double,double,double,ref):ref
       23.50 ( 1.50% of base) : SciMark\SciMark\SciMark.dasm - kernel:CopyMatrix(ref,ref)
       23.40 ( 1.61% of base) : Burgers\Burgers\Burgers.dasm - Burgers:GetCalculated2(int,int,double,double,double,ref):ref
       22.70 ( 3.74% of base) : Bytemark\Bytemark\Bytemark.dasm - NeuralJagged:do_mid_forward(int)
       22.65 ( 8.23% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:multiplyTo(ref,ref):this
Top method improvements by total perfscore change:
      -26.70 (-0.59% of base) : SciMark\SciMark\SciMark.dasm - SparseCompRow:matmult(ref,ref,ref,ref,ref,int)
      -13.80 (-1.41% of base) : BenchmarksGame\n-body\n-body-3\n-body-3.dasm - NBodySystem:.ctor():this
       -3.80 (-0.21% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:isProbablePrime(int):bool:this
       -2.70 (-0.20% of base) : SciMark\SciMark\SciMark.dasm - LU:solve(ref,ref,ref)
       -1.30 (-0.09% of base) : Bytemark\Bytemark\Bytemark.dasm - LUDecomp:lubksb(ref,int,ref,ref)
       -0.60 (-0.61% of base) : Benchstones\BenchI\Array1\Array1\Array1.dasm - Array1:VerifySort(ref):bool
       -0.20 (-0.33% of base) : Serialization\Deserialize\Deserialize.dasm - TestObject:Expected():ref
       -0.20 (-0.33% of base) : Serialization\Serialize\Serialize.dasm - TestObject:New():ref
       -0.10 (-0.01% of base) : SciMark\SciMark\SciMark.dasm - kernel:measureSparseMatmult(int,int,double,ref):double
       -0.10 (-0.10% of base) : Benchstones\BenchI\BubbleSort\BubbleSort\BubbleSort.dasm - BubbleSort:VerifySort(ref,int):bool
Top method regressions by perfscore change percentage:
       11.00 (14.15% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:getLowestSetBit():int:this
      156.30 ( 9.24% of base) : Benchstones\BenchF\SqMtx\SqMtx\SqMtx.dasm - SqMtx:Inner(ref,ref,int)
       21.40 ( 9.13% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:multiplyUpperTo(ref,int,ref):this
       22.65 ( 8.23% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:multiplyTo(ref,ref):this
       14.60 ( 7.96% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:modInt(int):int:this
      179.20 ( 7.93% of base) : Benchstones\BenchF\MatInv4\MatInv4\MatInv4.dasm - MatInv4:MProd(ref,ref,ref,byref)
       22.50 ( 6.26% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:multiplyLowerTo(ref,int,ref):this
       64.00 ( 5.76% of base) : Benchstones\BenchI\AddArray2\AddArray2\AddArray2.dasm - AddArray2:BenchInner1(ref,byref)
       45.50 ( 4.64% of base) : BenchmarksGame\fannkuch-redux\fannkuch-redux-5\fannkuch-redux-5.dasm - FannkuchRedux_5:firstPermutation(ref,ref,ref,int)
       36.10 ( 4.47% of base) : BenchmarksGame\fasta\fasta-1\fasta-1.dasm - Fasta_1:SelectNucleotides(ref,ref):ref
        3.60 ( 4.43% of base) : Bytemark\Bytemark\Bytemark.dasm - EMFloat:choose_nan(byref,byref,byref,int)
        3.60 ( 4.42% of base) : Bytemark\Bytemark\Bytemark.dasm - EMFloatClass:choose_nan(ref,ref,ref,int)
       22.40 ( 4.30% of base) : Bytemark\Bytemark\Bytemark.dasm - NeuralJagged:do_out_forward()
       53.60 ( 4.29% of base) : Burgers\Burgers\Burgers.dasm - Burgers:GetCalculated3(int,int,double,double,double,ref):ref
        6.30 ( 4.19% of base) : Bytemark\Bytemark\Bytemark.dasm - IDEAEncryption:en_key_idea(ref,ref)
       21.00 ( 4.12% of base) : Bytemark\Bytemark\Bytemark.dasm - NeuralJagged:do_mid_error()
       48.90 ( 3.80% of base) : BenchmarksGame\n-body\n-body-3\n-body-3.dasm - NBodySystem:Energy():double:this
       22.70 ( 3.74% of base) : Bytemark\Bytemark\Bytemark.dasm - NeuralJagged:do_mid_forward(int)
       10.40 ( 3.57% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:IntToString(int,int):ref
       22.40 ( 3.56% of base) : Bytemark\Bytemark\Bytemark.dasm - Neural:do_out_forward()
Top method improvements by perfscore change percentage:
      -13.80 (-1.41% of base) : BenchmarksGame\n-body\n-body-3\n-body-3.dasm - NBodySystem:.ctor():this
       -0.60 (-0.61% of base) : Benchstones\BenchI\Array1\Array1\Array1.dasm - Array1:VerifySort(ref):bool
      -26.70 (-0.59% of base) : SciMark\SciMark\SciMark.dasm - SparseCompRow:matmult(ref,ref,ref,ref,ref,int)
       -0.20 (-0.33% of base) : Serialization\Deserialize\Deserialize.dasm - TestObject:Expected():ref
       -0.20 (-0.33% of base) : Serialization\Serialize\Serialize.dasm - TestObject:New():ref
       -3.80 (-0.21% of base) : V8\Crypto\Crypto\Crypto.dasm - BigInteger:isProbablePrime(int):bool:this
       -2.70 (-0.20% of base) : SciMark\SciMark\SciMark.dasm - LU:solve(ref,ref,ref)
       -0.10 (-0.10% of base) : Benchstones\BenchI\BubbleSort\BubbleSort\BubbleSort.dasm - BubbleSort:VerifySort(ref,int):bool
       -1.30 (-0.09% of base) : Bytemark\Bytemark\Bytemark.dasm - LUDecomp:lubksb(ref,int,ref,ref)
       -0.10 (-0.01% of base) : SciMark\SciMark\SciMark.dasm - kernel:measureSparseMatmult(int,int,double,ref):double
53 total methods with score differences (10 improved, 43 regressed), 2002 unchanged.
3 files had text diffs but not score diffs.
Benchstones\BenchI\XposMatrix\XposMatrix\XposMatrix.dasm had 11 diffs
SIMD\SeekUnroll\SeekUnroll\SeekUnroll.dasm had 4 diffs
Benchstones\BenchF\Whetsto\Whetsto\Whetsto.dasm had 4 diffs
total perfscore diff: 1,648.25

@briansull
Copy link
Contributor Author

briansull commented Oct 4, 2019

@dotnet/jit-contrib PTAL

@BruceForstall
Copy link
Contributor

BruceForstall commented Oct 4, 2019

Is it useful to have "Top file regressions by perfScore" and "Top file improvements by perfScore"? Or is perfScore more useful as a per-function thing?

You've used both "perfScore" and "perfscore" in the output. I would prefer to stick with one, consistently. Maybe "PerfScore" instead.

@briansull
Copy link
Contributor Author

briansull commented Oct 4, 2019

Personally I find the per function information useful and I am fine with removing the per file perf scrore info.

BruceForstall
BruceForstall suggested changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

It looks like you did a massive copy/paste/edit of code size to create all the perfscore code. Is there a way to share these? E.g., make code size or perf score an argument or generic argument to functions/class, so there's no duplication?

src/jit-analyze/jit-analyze.cs Outdated
DisplayMethodMetric("\nTop method regressions by size (percentage):", methodRegressionCount, sortedMethodRegressionsByPercentage);
DisplayMethodMetric("\nTop method improvements by size (percentage):", methodImprovementCount, sortedMethodImprovementsByPercentage);
DisplayMethodMetric("\nTop method regressions by codesize change (bytes):", methodRegressionCount, sortedMethodRegressions);
DisplayMethodMetric("\nTop method improvements by codesize change (bytes):", methodImprovementCount, sortedMethodImprovements);
Copy link
Contributor

@BruceForstall BruceForstall Oct 4, 2019

Choose a reason for hiding this comment

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

I think you should write out "codesize" => "code size" and "perfscore" => "perf score" in the output

Copy link
Contributor Author

@briansull briansull Oct 5, 2019

Choose a reason for hiding this comment

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

I will write out "code size" and "PerfScore" in the output

@briansull
Copy link
Contributor Author

briansull commented Oct 5, 2019

Refactoring to use generics and interfaces instead of simple code duplication requires a lot more time.

@briansull
Use "PerfScore" uniformly
709995a 
remove "prolog bytes"
use "code size" instead of "codesize"
@briansull
A few more renames to PerfScore
fae1413
@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 17, 2019

I agree with Bruce here -- we will likely have other score-like metrics we want to aggregate/summarize in the future, and it would be better to generalize the pattern.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Oct 17, 2019

To be a bit more specific, I am thinking about jit-diff and jit-analyze support for things like dotnet/coreclr#18330 ... right now it is clunky to count things across a large swath of jitted methods.

@BruceForstall
Copy link
Contributor

BruceForstall commented Oct 23, 2019

I'll admit that looking at the duplication: my C# and LINQ knowledge is apparently not sophisticated enough to see an obvious code pattern to use to avoid the duplication. Anyone else have ideas here?

The question is whether we should let this go in as-is, hoping to get it refactored later, or if we want to ensure it gets refactored now, which might mean it doesn't go in because it won't be prioritized.

@briansull
Copy link
Contributor Author

briansull commented Nov 1, 2019

@BruceForstall Ping?

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Nov 1, 2019

I'll take a look at how we might avoid duplication -- though won't get to it until next week.

AndyAyersMS added a commit to AndyAyersMS/jitutils that referenced this pull request Nov 5, 2019
@AndyAyersMS
Update jit-analyze to handle multiple metrics
03d4f90 
Alternative take to dotnet#215 where jit-analyze is updated to look for
and process multiple bits of metric data.

With the pattern shown here it should be simple to add additional
metrics like the ones we already have.
AndyAyersMS added a commit to AndyAyersMS/jitutils that referenced this pull request Nov 5, 2019
@AndyAyersMS
Update jit-analyze to handle multiple metrics
9fc7651 
Alternative take to dotnet#215 where jit-analyze is updated to look for
and process multiple bits of metric data.

With the pattern shown here it should be simple to add additional
metrics like the ones we already have.
@AndyAyersMS AndyAyersMS mentioned this pull request Nov 5, 2019
Merged
AndyAyersMS added a commit that referenced this pull request Nov 8, 2019
@AndyAyersMS
Update jit-analyze to handle multiple metrics (#223)
1cbe62c 
Alternative take to #215 where jit-analyze is updated to look for
and process multiple bits of metric data. Default is still code size,
alternatives can be specified via `--metric <metricname>`

With the pattern shown here it should be simple to add additional
metrics like the ones we already have.

Updated `jit-diff` to also support the `--metric` option.
@BruceForstall
Copy link
Contributor

BruceForstall commented Nov 13, 2019

@briansull Is there anything from this PR you want to preserve (and submit as a new PR), since #223 has been merged, with some/all of this functionality?

@briansull
Copy link
Contributor Author

briansull commented Nov 13, 2019

I think so. I believe that the jit-diff changes are still need.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Nov 13, 2019

I believe that the jit-diff changes are still need.

I think I picked up all of your jit-diff changes in #223.

@CarolEidt
Copy link
Contributor

CarolEidt commented Nov 13, 2019

I think I picked up all of your jit-diff changes in #223.

I may be the source of the confusion. I wasn't able to find the option, but now I see that it's --metric.

@briansull
Copy link
Contributor Author

briansull commented Nov 14, 2019

I added a --count N option to jit-diffs to allow us to override the count value of jit-analyze directly from jit-diff

I didn't see that change.

@kunalspathak
Copy link
Contributor

kunalspathak commented Sep 9, 2020

Should we close this PR since #223 addressed most of the part of this PR?

@BruceForstall
Copy link
Contributor

BruceForstall commented Sep 9, 2020

Should we close this PR since #223 addressed most of the part of this PR?

It looks to me like we should. @briansull There is a --count N argument to jit-diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@BruceForstall BruceForstall BruceForstall requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@briansull @BruceForstall @AndyAyersMS @CarolEidt @kunalspathak