Skip to content

Don't call bit_width if value is nullptr in _Fmt_write #2097

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
Merged
StephanTLavavej merged 11 commits into from
Aug 27, 2021
Merged

Don't call bit_width if value is nullptr in _Fmt_write #2097

StephanTLavavej merged 11 commits into from
Aug 27, 2021

Conversation

@AZero13
Copy link
Contributor

@AZero13 AZero13 commented Aug 5, 2021

No description provided.

@AZero13 AZero13 requested a review from a team as a code owner August 5, 2021 22:19
jovibor
jovibor reviewed Aug 5, 2021
View reviewed changes
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Aug 6, 2021
AdamBucior
AdamBucior reviewed Aug 7, 2021
View reviewed changes
@AZero13
Copy link
Contributor Author

AZero13 commented Aug 7, 2021

Done!

@AZero13
Copy link
Contributor Author

AZero13 commented Aug 7, 2021

Also this is technically a performance patch since we do the check anyway, but now moving it first before we call the function in the event value is nullptr

@StephanTLavavej StephanTLavavej added performance Must go faster and removed enhancement Something can be improved labels Aug 8, 2021
miscco
miscco approved these changes Aug 9, 2021
View reviewed changes
@AZero13
Copy link
Contributor Author

AZero13 commented Aug 11, 2021

@miscco done!

barcharcraz
barcharcraz approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

This looks correct, but I'm not 100% convinced it's a performance improvement except in debug mode. But debug perf is important too, so I'm fine with it.

barcharcraz
barcharcraz reviewed Aug 16, 2021
View reviewed changes
@barcharcraz barcharcraz removed their assignment Aug 16, 2021
CaseyCarter
CaseyCarter suggested changes Aug 16, 2021
View reviewed changes
@CaseyCarter CaseyCarter self-assigned this Aug 16, 2021
@AZero13
Copy link
Contributor Author

AZero13 commented Aug 17, 2021

Issues addressed!

@AZero13 AZero13 requested a review from CaseyCarter August 17, 2021 18:39
@AlexGuteniev AlexGuteniev mentioned this pull request Aug 18, 2021
Closed
@AZero13
Copy link
Contributor Author

AZero13 commented Aug 23, 2021

So when can we merge, if I may ask?

@celonymire
Copy link
Contributor

celonymire commented Aug 23, 2021

So when can we merge, if I may ask?

They usually merge multiple PRs at the same time, so it may take a while.

jrtc27
jrtc27 reviewed Aug 24, 2021
View reviewed changes
@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 26, 2021

I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed.

(As @sam20908 mentioned, the mirroring process is semi-manual, and requires enough effort that we batch up PRs in order to save dev time. Your PR will be part of the next batch of 17 PRs. There's no set schedule for mirroring, although it tends to be completed at the end of each week.)

@StephanTLavavej StephanTLavavej merged commit c1920e3 into microsoft:main Aug 27, 2021
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 27, 2021

Thanks for improving this codepath! 🚀 🎉 😸

@AZero13 AZero13 deleted the Width branch August 30, 2021 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+7 more reviewers

@jrtc27 jrtc27 jrtc27 left review comments

@jovibor jovibor jovibor left review comments

@celonymire celonymire celonymire left review comments

@AdamBucior AdamBucior AdamBucior left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@barcharcraz barcharcraz barcharcraz approved these changes

@miscco miscco miscco approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@AZero13 @celonymire @StephanTLavavej @CaseyCarter @jrtc27 @barcharcraz @miscco @jovibor @AdamBucior