-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
Remove the property disabling optimization #19701
Remove the property disabling optimization #19701
Conversation
- Remove the property group that disabled optimization when building for non-Windows platforms - Set packing size on the `UnixTm` struct to match our native declaration - Add `SetLastError` to our p/invoke as libpsl-native!setdate does set `errno`
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
As remainder #6881 (comment) |
Is the issue you're referring to something else that also only occurs when optimization is off? Or is it a different thing we also need to fix in the same area? Asking because for this PR I'm mainly looking at getting optimization back on. Side note, do you happen to know a good resource going in depth on the layout differences for ARM? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider backporting this to 7.3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SeeminglyScience!
I pointed the issue to inform you about history. The reason for this problem for many years is that we use a complex structure when we might not. So
I didn't need to dive deep, so I just watched how it was done in the .Net Runtime.
|
Can you share any benchmark measurements to show how much improvements do we get on linux? |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
It used to fail on macOS and according to @SeeminglyScience it worked this time. So, we will merge this PR and keep an eye on the release automation test run. If a failure happens there on a specific platform, that will tell us where to look. |
Did you get a chance to measure the improvements out of it? |
No, we didn't do measurement. But it should be faster and more efficient according to the documentation of this compiler option. |
Not yet but I do plan to. Busy with some other tasks atm but when I get around to it I'll share in this thread |
🎉 Handy links: |
PR Summary
NOTE: I'm mainly opening this PR to see if tests fail on any of the platforms we have CI for. If it does, that still tells us where to look.
UnixTm
struct to match our native declarationSetLastError
to our p/invoke aslibpsl-native!setdate
does seterrno
(without this, I was getting "Unknown Error 203" when runningSet-Date
without sudo. With the change, it displays the expected error)Resolves #19677
PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).