Skip to content

[CompilerPerf] Extends #5112 ideas to Comparers (i.e >, >=, <, <= and LanguagePrimitives.FastGenericComparer) - #5278

Closed
manofstick wants to merge 38 commits into
dotnet:masterfrom
manofstick:nobox_comparer
Closed

[CompilerPerf] Extends #5112 ideas to Comparers (i.e >, >=, <, <= and LanguagePrimitives.FastGenericComparer)#5278
manofstick wants to merge 38 commits into
dotnet:masterfrom
manofstick:nobox_comparer

Conversation

@manofstick

Copy link
Copy Markdown
Contributor

This is a standalone improvement, as is #5112, but this is branched off the tail of that PR as I see it being approved and integrated first, as this is still currently a WIP.

Comment threadsrc/fsharp/FSharp.Core/prim-types.fs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not if...then...else?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

that's what was there previously...

Comment threadsrc/fsharp/FSharp.Core/prim-types.fs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sbyte[] will match on this branch as well. Are you sure GenericHashByteArray will produce correct result in this case?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yep, I know, still WIP...

Comment threadsrc/fsharp/FSharp.Core/prim-types.fs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This whole condition is fully duplicated on line 1152. Maybe extract it into a function?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

not the same - strings

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I wouldn't bother too much checking this yet. I'm still a while off completing it...

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

this is done in the final PR #5307 btw, so assuming the whole chain of PRs is accepted...

@vasily-kirichenko

Copy link
Copy Markdown
Contributor

@manofstick merge conflict!

@manofstick

manofstick commented Jul 13, 2018

Copy link
Copy Markdown
ContributorAuthor

@vasily-kirichenko

Not being the master of git (or github) I don't know the process to fix.

The conflicted code can just be deleted (it was a line you added, which is now redundant). So the solution is to just ignore the mainline.

Should I could rebase off master?

@manofstick

Copy link
Copy Markdown
ContributorAuthor

Slightly modified tests from #5112... (so they don't necessarily make sense, more just to try as many cases...)

1) https://gist.github.com/manofstick/33fefb8ac6b60d401375be738b521ac3

64-bit

testoriginalmodifiedpercent
custom dynamic3599.001193.1733%
custom structural3614.001380.3338%
custom default1190.831197.33101%
value dynamic2958.332187.1774%
value structural1248.501240.0099%
value default1245.331249.00100%
gen value dynamic6026.332466.5041%
gen value structural4130.171503.3336%
gen value default4085.831524.8337%
ref dynamic2680.831749.6765%
ref structural1214.001262.83104%
ref default1314.671361.67104%
gen ref dynamic5882.172026.1734%
gen ref structural4343.501572.3336%
gen ref default4502.501673.5037%
tuple dynamic5618.674430.0079%
tuple structural1099.671070.3397%
tuple default7953.007844.3399%
value tuple dynamic5719.001570.1727%
value tuple structural5835.831558.8327%
value tuple default1686.671535.1791%

32-bit

testoriginalmodifiedpercent
custom dynamic11149.331192.3311%
custom structural11198.003798.534%
custom default1280.501208.8394%
value dynamic9668.333648.8338%
value structural1337.001251.3394%
value default1321.831259.8395%
gen value dynamic19472.336444.8333%
gen value structural11084.003522.8332%
gen value default10975.003560.6732%
ref dynamic10149.173766.8337%
ref structural1353.501281.6795%
ref default1505.671438.8396%
gen ref dynamic20249.336484.8332%
gen ref structural12903.834804.1737%
gen ref default11318.673961.535%
tuple dynamic18689.6711176.3360%
tuple structural1326.331256.6795%
tuple default9330.178640.8393%
value tuple dynamic17674.6713488%
value tuple structural17620.831336.838%
value tuple default1439.331349.594%

@manofstick

Copy link
Copy Markdown
ContributorAuthor

2) https://gist.github.com/manofstick/04ba0c70c398bf5edeaba9b17d0b17c5

testoriginalmodifiedpercent
mapTest 32-bit18169335618%
mapTest 64-bit7414444160%

@manofstick

Copy link
Copy Markdown
ContributorAuthor

3) https://gist.github.com/manofstick/bd374436a6d40218b2d836db34c494ca

testoriginalmodifiedpercent
32-bit771676827889%
64-bit1429497461552%

@manofstick

Copy link
Copy Markdown
ContributorAuthor

4) https://gist.github.com/manofstick/258edf7e7d39a76e9d1cdf0316ae2b47

testoriginalmodifiedpercent
non-generic 32-bit563602454544%
generic 32-bit1178334642139%
non-generic 64-bit236511822777%
generic 64-bit447382205149%

@manofstick

Copy link
Copy Markdown
ContributorAuthor

5) https://gist.github.com/manofstick/847922bcce2e2f47d3eca033ed9dc068

testoriginalmodifiedpercent
32-bit897.436340%
64-bit499.2306.861%

@manofstick

Copy link
Copy Markdown
ContributorAuthor

6) https://gist.github.com/manofstick/b4fa5acc1abe3ca73a77a7eea12d205a

testoriginalmodifiedpercent
32-bit9707.8575.86%
64-bit3733.8498.813%

@manofstickmanofstick changed the title [WIP] [CompilerPerf] Extends #5112 ideas to Comparers (i.e >, >=, <, <= and LanguagePrimitives.FastGenericComparer)[CompilerPerf] Extends #5112 ideas to Comparers (i.e >, >=, <, <= and LanguagePrimitives.FastGenericComparer)Jul 13, 2018
@manofstick

Copy link
Copy Markdown
ContributorAuthor

@dsyme - I think this is done.

manofstick added a commit to manofstick/visualfsharp that referenced this pull request Aug 7, 2018
manofstick added a commit to manofstick/visualfsharp that referenced this pull request Aug 7, 2018
manofstick added a commit to manofstick/visualfsharp that referenced this pull request Aug 12, 2018
@KevinRansom

Copy link
Copy Markdown
Contributor

@dsyme , @manofstick , @cartermp -- what do we wnt to do with this PR? It looks beneficial, but the momentum just died on it!

Should I close it, or will we revisit it in the near future?

Thanks

Kevin

@KevinRansom

Copy link
Copy Markdown
Contributor

@manofstick you will still do these really cool PR's in the future right, I learn a ton looking through them :-)

@manofstick

Copy link
Copy Markdown
ContributorAuthor

@KevinRansom ha! well that's good! if I do have any more ideas I'll give 'em a crack, but I think most of my ideas are a bit too radical to get slurped upstream...

@KevinRansom

Copy link
Copy Markdown
Contributor

@manofstick , I am not so sure, I just think the rate of change of some stuff is so slow that FUD and real life(TM) gets in the way.

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

Labels

Area-LibraryIssues for FSharp.Core not covered elsewhere

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@manofstick@vasily-kirichenko@KevinRansom@TIHan@dsyme