Skip to content

Use memset in even more cases #1273

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 17 commits into from
Oct 29, 2020
Merged

Use memset in even more cases #1273

StephanTLavavej merged 17 commits into from
Oct 29, 2020

Conversation

@AdamBucior
Copy link
Contributor

@AdamBucior AdamBucior commented Sep 5, 2020 •
edited
Loading

Another small PR to optimize (ranges::)(uninitialized_)fill(_n). I applied the optimization only to the _HAS_IF_CONSTEXPR branch because without if constexpr it becomes very tricky to implement and I don't know how long the !_HAS_IF_CONSTEXPR branch will live. Part of #431.

@AdamBucior AdamBucior requested a review from a team as a code owner September 5, 2020 10:56
@CaseyCarter CaseyCarter added the performance Must go faster label Sep 5, 2020
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 23, 2020

I pushed a merge to uninitialized_fill_n() in <memory> - it needed a bit of manual surgery, and I think I got it right, but please double-check.

@AdamBucior
Copy link
Contributor Author

AdamBucior commented Sep 23, 2020

I pushed a merge to uninitialized_fill_n() in <memory> - it needed a bit of manual surgery, and I think I got it right, but please double-check.

Looks good

CaseyCarter
CaseyCarter reviewed Sep 24, 2020
View reviewed changes
@AdamBucior @CaseyCarter
static_assert is_scalar
4b0f4cd 
Co-authored-by: Casey Carter <cartec69@gmail.com>
AdamBucior
AdamBucior commented Sep 24, 2020
View reviewed changes
AdamBucior
AdamBucior commented Sep 24, 2020
View reviewed changes
CaseyCarter
CaseyCarter approved these changes Oct 3, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks for the extra test coverage.

@CaseyCarter CaseyCarter removed their assignment Oct 3, 2020
@StephanTLavavej StephanTLavavej self-assigned this Oct 7, 2020
StephanTLavavej
StephanTLavavej requested changes Oct 28, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this optimization! I found one bug involving floating-point negative zero, but otherwise this looks very solid.

@StephanTLavavej StephanTLavavej removed their assignment Oct 28, 2020
AdamBucior and others added 2 commits October 28, 2020 09:01
@AdamBucior @StephanTLavavej
Apply suggestions from code review
599542e 
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@AdamBucior
Code review
0048d22
@StephanTLavavej StephanTLavavej self-assigned this Oct 28, 2020
@StephanTLavavej
Avoid Clang's -Wzero-as-null-pointer-constant warning.
3a937b3 
`static_cast<_Ty>(0)` emits this warning for object pointers, function
pointers, and `nullptr_t`. We can avoid this warning by using empty
braces to request zero-initialization.
@StephanTLavavej
Portably test for negative zero.
2866b64
@StephanTLavavej
Test allocate_shared<int[]>.
3d21ed4
@StephanTLavavej
Merge branch 'master' into gh1273_memset
d3fd586
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 29, 2020

Looks great! I've pushed three small changes (FYI @CaseyCarter) and merged with master (without conflicts), so I can add your PR to today's batch for merging.

  • Avoid Clang's -Wzero-as-null-pointer-constant warning.
    • static_cast<_Ty>(0) emits this warning for object pointers, function pointers, and nullptr_t. We can avoid this warning by using empty braces to request zero-initialization.
  • Portably test for negative zero.
    • We conventionally avoid using internal machinery in tests unless it allows something to be tested that would be difficult otherwise. Instead of _Bit_cast, we can use <cmath>'s signbit() to detect negative zero.
  • Test allocate_shared<int[]>.

@StephanTLavavej StephanTLavavej merged commit a27d894 into microsoft:master Oct 29, 2020
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 29, 2020

Thanks again for this optimization, automatically making user code faster! This will ship in VS 2019 16.9 Preview 2. 🎉

@AdamBucior AdamBucior deleted the fill_zero_optimization branch October 29, 2020 08:53
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

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter 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.

4 participants

@AdamBucior @StephanTLavavej @CaseyCarter @barcharcraz