Skip to content
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.

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

Update web.csproj #6535

Merged
merged 2 commits into from
Jul 22, 2023
Merged

Update web.csproj #6535

merged 2 commits into from
Jul 22, 2023

Conversation

ShreyasJejurkar
Copy link
Contributor

Enable PGO

Copy link
Collaborator

@waghanza waghanza left a comment

Choose a reason for hiding this comment

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

what is PGO ?

@ShreyasJejurkar
Copy link
Contributor Author

It stands for Profile guided optimization! In case if you are not aware -> https://en.wikipedia.org/wiki/Profile-guided_optimization

Copy link
Collaborator

@waghanza waghanza left a comment

Choose a reason for hiding this comment

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

seems interesting.

is it something that is production-ready / recommended for production ?

is it something that could be applied on other csharp frameworks ?

@ShreyasJejurkar
Copy link
Contributor Author

Yes, of course it's production ready. We can apply that to all projects. Moreover in .NET 8 it's already on by default. So we can merge this PR in short.

I also want to enable one more option, but I would be doing that with next PR.

Copy link
Collaborator

@waghanza waghanza left a comment

Choose a reason for hiding this comment

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

then please replicate this option on all frameworks :-)

could it also be used on F# ? or not at all ?

@ShreyasJejurkar
Copy link
Contributor Author

It should work on F# as well, but I really never tried. I guess we can get this merge to C# first and then we can try F# after that!

@ShreyasJejurkar
Copy link
Contributor Author

then please replicate this option on all frameworks :-)

Let the authors of the FW take care of them, I just made it on official aspnetcore framework!

@cyrusmsk
Copy link
Contributor

This is what I asked before @waghanza
Do all kind of optimizations (like LTO, PGO, switched off checks) are allowed?

@waghanza
Copy link
Collaborator

Not sure then why this reply was made #6488 (comment).

I'm honestly not the best person to say if this is legitimate or not

wdyt @dom96 @panesofglass @adamdruppe @itsumura-h ?

@adamdruppe
Copy link

PGO is ok. My comment was specifically about D's horrible -release switch - which is independent of the optimization switches, etc - which strips out array bound checking (which is important for preventing a common source of security holes, technically you are never supposed to go out of bounds anyway, but nobody intentionally writes security holes and you want defense in depth) and disables asserts in a way that can lead to undefined behavior.

But PGO, LTO, -O2, stuff like that, are entirely separate (in fact, D's broken -release switch doesn't even actually enable any of those things!) and those are fine to use.

@ShreyasJejurkar
Copy link
Contributor Author

I understand what you guys are suspecting here like hacky optimization and things. But let me clear one thing enabling PGO is not a hacky optimization technique, it's the compiler-level static optimization so nothing is odd there! Moreover, Dynamic PGO is by default going forward with .NET 8 (dotnet/runtime#86225), so nothing is hacky here!

@waghanza
Copy link
Collaborator

Ok, then. This was specific to D

@waghanza
Copy link
Collaborator

et the authors of the FW take care of them, I just made it on official aspnetcore framework!

Not sure we should. My point is that we can merge this, if it's supported by C# compiler + it is a best practice, let's see it this affect negatively performances

@ShreyasJejurkar
Copy link
Contributor Author

I don't think it will degrade perf, moreover, it will improve it by far! Just look at the linked issues here at the end of this dotnet/runtime#86225 😅😅

@ShreyasJejurkar
Copy link
Contributor Author

I guess we are ready to merge this unless someone has doubts about this!

@waghanza
Copy link
Collaborator

I have not doubt about this configuration, but I would prefer to replicate it on each frameworks

Not sure we will use .NET 8, I prefer to keep with LTS

@waghanza waghanza mentioned this pull request Jul 22, 2023
@waghanza waghanza merged commit d615bc4 into the-benchmarker:master Jul 22, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants