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.

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

Don't update entries in TypedArena if T does not need drop #78144

Merged
merged 1 commit into from
Oct 20, 2020
Merged

Don't update entries in TypedArena if T does not need drop #78144

merged 1 commit into from
Oct 20, 2020

Conversation

bugadani
Copy link
Contributor

As far as I can tell, entries is only used when dropping TypedArenaChunks and their contents. It is already ignored there, if T is not mem::needs_drop, this PR just skips updating it's value.

You can see TypedArenaChunk ignoring the entry count in L71. The reasoning is similar to what you can find in DroplessArena.

r? @oli-obk

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

oli-obk commented Oct 20, 2020

Hmm, this makes me wonder whether we could make the entries field actually be () in case T doesn't need dropping. Any mutations on the field would then require a trait method that in the !needs_drop case just doesn't do anything.

This way we don't get into situations like this where we accidentally use a field that is never touched.

Maybe this is overkill? But it feels kind of nice to me. What do you think?

@bugadani
Copy link
Contributor Author

bugadani commented Oct 20, 2020 •
edited
Loading

I don't think I want to make TypedArena any more complicated, just to save 4-8 bytes in like 3 (well, the query system makes it actually hard to say, how many) places. I think I've already introduced enough complexity :) But I can try and think of a solution if complexity is not a huge concern.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

The bytes aren't really the issue here, but the uses of the field. Though I guess you're right about complexity.

So... I guess leave a comment on that if condition stating why we can ignore this field here safely (because it's never read unless that condition is true).

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

@bors r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Oct 20, 2020

📌 Commit 2705cae has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2020
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 20, 2020
@GuillaumeGomez
Rollup merge of rust-lang#78144 - bugadani:elements-nodrop, r=oli-obk
5b2a3fc 
Don't update `entries` in `TypedArena` if T does not need drop

As far as I can tell, `entries` is only used when dropping `TypedArenaChunk`s and their contents. It is already ignored there, if T is not `mem::needs_drop`, this PR just skips updating it's value.

You can see `TypedArenaChunk` ignoring the entry count in L71. The reasoning is similar to what you can find in `DroplessArena`.

r? @oli-obk
This was referenced Oct 20, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2020
@bors
Auto merge of rust-lang#78162 - GuillaumeGomez:rollup-6a4qiqu, r=Guil…
31530e5 
…laumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#78046 (Add codegen test for issue rust-lang#73827)
 - rust-lang#78061 (Optimize const value interning for ZST types)
 - rust-lang#78070 (we can test std and core panic macros together)
 - rust-lang#78076 (Move orphan module-name/mod.rs files into module-name.rs files)
 - rust-lang#78129 (Wrapping intrinsics doc links update.)
 - rust-lang#78133 (Add some MIR-related regression tests)
 - rust-lang#78144 (Don't update `entries` in `TypedArena` if T does not need drop)
 - rust-lang#78145 (Drop unneeded `mut`)
 - rust-lang#78157 (Remove unused type from librustdoc)

Failed merges:

r? `@ghost`
@bors bors merged commit ad218f9 into rust-lang:master Oct 20, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 20, 2020
@bugadani bugadani deleted the elements-nodrop branch October 21, 2020 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
5 participants
@bugadani @oli-obk @bors @rust-highfive @rustbot