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

Remove Box from RcSlice #51281

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Mark-Simulacrum
Member

Mark-Simulacrum commented Jun 1, 2018

This isn't necessary as Rc/Arc already have an internal heap allocation,
so the additional Box represented a useless pointer dereference.

Remove Box from RcSlice
This isn't necessary as Rc/Arc already have an internal heap allocation,
so the additional Box represented a useless pointer dereference.
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Jun 1, 2018

r? @petrochenkov

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

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jun 1, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Jun 1, 2018

Nice! I actually looked at this myself because these allocations were showing up as moderately hot in some profiles, but I managed to convince myself they were unavoidable O_o

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jun 5, 2018

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Jun 5, 2018

@eddyb

This comment has been minimized.

Member

eddyb commented Jun 6, 2018

Note that there is a cost here of moving the contents into an Rc.
r=me if that's accounted for and this is a net positive.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jun 6, 2018

@bors try

bors added a commit that referenced this pull request Jun 6, 2018

Auto merge of #51281 - Mark-Simulacrum:no-box-rcslice, r=<try>
Remove Box from RcSlice

This isn't necessary as Rc/Arc already have an internal heap allocation,
so the additional Box represented a useless pointer dereference.
@bors

This comment has been minimized.

Contributor

bors commented Jun 6, 2018

⌛️ Trying commit 79010c2 with merge d9589e4...

@bors

This comment has been minimized.

Contributor

bors commented Jun 6, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jun 6, 2018

@nnethercote This does seem to be a regression in wall time and memory usage.. https://perf.rust-lang.org/compare.html?start=685faa2c3ee062aaf30ddf6d3daf4ef3b576979b&end=d9589e4c2db96769a2be22f29625cb31266557cc&stat=max-rss. I guess maybe we shouldn't land this? Is your interpretation different?

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Jun 7, 2018

I took a look at deep-vector and regex with Cachegrind and DHAT. deep-vector because it showed the big regression, and regex because DHAT shows its the benchmark that has the highest proportion of these RcSlice allocations.

DHAT agrees that the max RSS does increase significantly for deep-vector. (I get a 16% increase on a clean-check build.) Presumably because we end up having two copies of the data live while copying from the Vector to the slice.

Other than that, there's not much change -- regex is hardly affected -- and no clear benefit. The total number of allocations done doesn't change. Instruction counts don't change.

It's a shame, but I agree that this shouldn't be landed.

@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Jun 7, 2018

Thanks for verifying!

@Mark-Simulacrum Mark-Simulacrum deleted the Mark-Simulacrum:no-box-rcslice branch Jun 7, 2018

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