KEMBAR78
Add `std::fill` and `std::fill_n` benchmarks by barcharcraz · Pull Request #5400 · microsoft/STL · GitHub
Skip to content

Conversation

@barcharcraz
Copy link
Contributor

I wrote these benchmarks while fixing some issues with memset loop recognition in the compiler, but I think they are more broadly useful.

@barcharcraz barcharcraz requested a review from a team as a code owner April 11, 2025 01:01
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews Apr 11, 2025
@StephanTLavavej StephanTLavavej added the test Related to test code label Apr 11, 2025
@StephanTLavavej StephanTLavavej self-assigned this Apr 11, 2025
@StephanTLavavej
Copy link
Member

Thanks! 😸 I pushed significant changes - please meow if you have concerns.

@StephanTLavavej StephanTLavavej removed their assignment Apr 15, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in STL Code Reviews Apr 15, 2025
@barcharcraz
Copy link
Contributor Author

nah, lgtm.
However:

I don't really like pulling the allocators out into a template parameter, as I don't think it's worth multiplying the test matrix for them (although having some benchmarks use one and some the other probably was bad).

@AlexGuteniev
Copy link
Contributor

I don't really like pulling the allocators out into a template parameter, as I don't think it's worth multiplying the test matrix for them (although having some benchmarks use one and some the other probably was bad).

If there's little difference in performance between allocators, I'd just use the worse one (not_highly_aligned_allocator), to make AVX2 or AVX-512 vectorization, if they exist here, of less advantage.

@StephanTLavavej StephanTLavavej changed the title add std::fill benchmarks Add std::fill and std::fill_n benchmarks Apr 16, 2025
@StephanTLavavej
Copy link
Member

Sounds good to me since I saw no perf difference. I'll validate and push.

@barcharcraz
Copy link
Contributor Author

lgtm

@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews Apr 22, 2025
@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 cac14a0 into microsoft:main Apr 22, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from Merging to Done in STL Code Reviews Apr 22, 2025
@StephanTLavavej
Copy link
Member

Thanks for filling out our benchmark directory! ⏱️ 📈 😹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Related to test code

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants