Skip to content

Don't copy into arena when we had to heap allocate for iter #78067

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

Closed
wants to merge 1 commit into from

Conversation

bugadani
Copy link
Contributor

Based on #77945 (comment)

If we already had to heap-allocate for an iterator, let's use that memory instead of copying the contents into the arena. Perf could go either way, but hoping for the best 🤞

cc @est31 since this is your idea (although I did not understand your comment at the time, and I basically came up with the same)

@rust-highfive
Copy link
Contributor

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2020
@bugadani
Copy link
Contributor Author

I think I would prefer some sort of linked list instead of a vector, so that growing that won't re-alloc, but this should be a good enough first approximation of what can be done with this idea. If the tests pass 😅

@est31
Copy link
Member

est31 commented Oct 18, 2020

@bors try

@bors
Copy link
Collaborator

bors commented Oct 18, 2020

⌛ Trying commit 8ee450feba5e33ed80217daab3034265cfef4edc with merge c2dea8a743324c28c48d675cb0373cb75d3161f7...

@bors
Copy link
Collaborator

bors commented Oct 18, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: c2dea8a743324c28c48d675cb0373cb75d3161f7 (c2dea8a743324c28c48d675cb0373cb75d3161f7)

@jonas-schievink
Copy link
Contributor

@rust-timer build c2dea8a743324c28c48d675cb0373cb75d3161f7

@rust-timer
Copy link
Collaborator

Queued c2dea8a743324c28c48d675cb0373cb75d3161f7 with parent ad268bd, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c2dea8a743324c28c48d675cb0373cb75d3161f7): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants