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.

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

Implement Events::extend with Vec::extend #2149

Closed

Conversation

NathanSWard
Copy link
Contributor

Problem:

  • Events::extend manually called Vec::push for each element of the
    provided iterator.
  • This can potentially be an expensive.

Solution:

  • Use Vec::extend over a Vec::push for loop.

Fixes: #2146

Blocked by: #2145

alice-i-cecile and others added 20 commits May 11, 2021 16:23
Moves impl blocks directly after types, organizes like-types together, presents shared functionality first
Also added handy drain_with_id method
Event buffer's state should not clash with State concept
Direct copy of into_inner for Mut
Consistency with event_consumer and improves simplicity
Required to ensure Event readers continue functioning correctly
Co-authored-by: Nathan Ward <43621845+NathanSWard@users.noreply.github.com>
Co-authored-by: Nathan Ward <43621845+NathanSWard@users.noreply.github.com>
@NathanSWard
Copy link
Contributor Author

This is based on #2145 so the only change this PR adds is the final commit 67afc54

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels May 12, 2021
@alice-i-cecile
Copy link
Member

So, the main question that I have here is should we be benchmarking this, and if so, how?

Creating a proper events benchmark would be nice for #2073 as well, if we can design something that works for both.

Problem:
- Events::extend manually called Vec::push for each element of the
  provided iterator.
- This can potentially be an expensive.

Solution:
- Use Vec::extend over a Vec::push for loop.
@NathanSWard
Copy link
Contributor Author

Closing this in favor of a change that's coalescing this and some Event bug fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events should use .extend internally
2 participants