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.

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

Optimize fiopen #2095

Merged
merged 4 commits into from
Mar 4, 2022
Merged

Optimize fiopen #2095

merged 4 commits into from
Mar 4, 2022

Conversation

AreaZR
Copy link
Contributor

@AreaZR AreaZR commented Aug 5, 2021

Also the if statements are now consistent with returning nullptr now.

@AreaZR AreaZR requested a review from a team as a code owner August 5, 2021 19:04
@CaseyCarter CaseyCarter added the performance Must go faster label Aug 5, 2021
CaseyCarter
CaseyCarter reviewed Aug 5, 2021
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@AreaZR AreaZR changed the title Optimize fiopen by reducing jumps Optimize fiopen by checking mode == 0 before looping Aug 5, 2021
@AreaZR AreaZR requested a review from CaseyCarter August 5, 2021 19:40
@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 5, 2021

All issues addressed!

AlexGuteniev
AlexGuteniev reviewed Aug 6, 2021
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 8, 2021

Done!

AlexGuteniev
AlexGuteniev reviewed Aug 8, 2021
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 8, 2021

Addressed!

@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 13, 2021

Addressed!

@AreaZR AreaZR changed the title Optimize fiopen by checking mode == 0 before looping Optimize fiopen by doing an early return if mode == 0 Aug 15, 2021
AlexGuteniev
AlexGuteniev suggested changes Aug 16, 2021
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 16, 2021

Positive case is actually NOT less optimal.

Negative case ALWAYS has to be checked first because otherwise it will cause a bug if mode == 0

CaseyCarter
CaseyCarter requested changes Aug 16, 2021
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
StephanTLavavej
StephanTLavavej reviewed Aug 17, 2021
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@AreaZR
Copy link
Contributor Author

AreaZR commented Aug 21, 2021

Addressed!

@AreaZR
Copy link
Contributor Author

AreaZR commented Oct 3, 2021

Any updates?

@AlexGuteniev
Copy link
Contributor

Any updates?

Seing other issues in maintainers plan helps having some patience https://github.com/microsoft/STL/projects/6

@AreaZR AreaZR changed the title Optimize fiopen by doing an early return if mode == 0 Optimize fiopen Nov 4, 2021
CaseyCarter
CaseyCarter requested changes Nov 8, 2021
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the affects redist Results in changes to separately compiled bits label Dec 15, 2021
@AreaZR @CaseyCarter
Update stl/src/fiopen.cpp
536dc3a 
Co-authored-by: Casey Carter <[email protected]>
CaseyCarter
CaseyCarter approved these changes Feb 16, 2022
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter removed their assignment Feb 16, 2022
CaseyCarter
CaseyCarter reviewed Feb 16, 2022
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
StephanTLavavej
StephanTLavavej requested changes Feb 16, 2022
View reviewed changes
stl/src/fiopen.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks - looks good to me, I believe this is ready to merge. However, due to the VC Redist Lockdown, we'll need to hold off on merging this for a hopefully short time, until internal testing with the "locked redist" is added (currently being investigated by @MahmoudGSaleh). Once that's in place, we can merge this change as it affects msvcp140.dll but does not require synchronized changes to the headers.

@StephanTLavavej StephanTLavavej added the blocked Something is preventing work on this label Feb 18, 2022
@StephanTLavavej StephanTLavavej removed the blocked Something is preventing work on this label Mar 2, 2022
@StephanTLavavej StephanTLavavej self-assigned this Mar 3, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit f585110 into microsoft:main Mar 4, 2022
@StephanTLavavej
Copy link
Member

Thanks for simplifying this code and eliminating the weird zero-termination of that array! 🎉 🚀 😺

@AreaZR AreaZR deleted the CLion branch September 26, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@AlexGuteniev AlexGuteniev AlexGuteniev requested changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
affects redist Results in changes to separately compiled bits performance Must go faster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AreaZR @AlexGuteniev @StephanTLavavej @CaseyCarter