Use Rc<str> and Rc<[T]> instead of Rc<String> and Rc<Vec<T>> #49645
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bors try I'd like to check the perf. cc @Mark-Simulacrum |
bors
added a commit
that referenced
this pull request
Apr 4, 2018
Use Rc<str> and Rc<[T]> instead of Rc<String> and Rc<Vec<T>> to save a bit of memory and pointer chasing. For the `Vec<T>` case, there is one instance each of `get_mut` and `make_mut` that preclude the use of the slice type: * the `get_mut` one does `push`, so I left it as is * the `make_mut` one does a `reverse`, which would be fine but `Lrc<[T]>` does not impl `Clone` (should it?) - however, the reverse seems to be unnecessary - please if the solution makes sense Had some unrelated-looking local test failures, let's see if if CI gets them too.
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@TimNN Questionable fragment ;) Fixing the test now... |
bjorn3
reviewed
Apr 4, 2018
| @@ -2341,14 +2341,14 @@ impl<'a, K, V> OccupiedEntry<'a, K, V> { | |||
| /// use std::collections::hash_map::{Entry, HashMap}; | |||
| /// use std::rc::Rc; | |||
| /// | |||
| /// let mut map: HashMap<Rc<String>, u32> = HashMap::new(); | |||
| /// map.insert(Rc::new("Stringthing".to_string()), 15); | |||
| /// let mut map: HashMap<Rc<str>, u32> = HashMap::new(); | |||
There was a problem hiding this comment.
I think we shouldn't promote use of Rc<String>, rather show that Rc<str> is now feasible and preferable.
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The try build is successful. Again, the reply is missing, probably because new commits are pushed in between. Anyway, please schedule |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
pietroalbini
added
the
S-waiting-on-author
|
Try build queued for perf. |
nikomatsakis
approved these changes
Apr 4, 2018
|
r? @steveklabnik for the changes to hashmap example etc |
kennytm
added
S-waiting-on-review
|
Ignoring the spurious |
|
☔ The latest upstream changes (presumably #49696) made this pull request unmergeable. Please resolve the merge conflicts. |
There is one instace where Lrc::get_mut().unwrap() is used on the reference to make a push, this cannot be converted. One instance of Lrc::make_mut() was removed -- the side effect of changing the original vector cannot have mattered since make_mut() might have cloned it.
|
Updated. |
|
☔ The latest upstream changes (presumably #49672) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hmm. I'm torn on this change. I wouldn't expect a real performance win -- as we saw on perf, this is basically in the noise, which is what I would expect. (Note also that even when judged from a "micro-efficiency" perspective, this is not an obvious win: My bigger concern though is that constructing a Note all that we don't really get much increased flexibility here. e.g.,, given a So in summary:
cc @rust-lang/compiler -- thoughts? |
|
👍 for elegance |
Yeah - you need |
|
Actually I would have expected a performance loss because constructing a |
|
I've been wanting to get rid of the |
|
Ping from triage! What's the status of this PR? |
1 similar comment
|
Ping from triage! What's the status of this PR? |
|
Looks like the change is not unanimous, and a significant part is likely to be refactored away, so I'll close it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
to save a bit of memory and pointer chasing.
For the
Vec<T>case, there is one instance each ofget_mutandmake_mutthat preclude the use of the slice type:get_mutone doespush, so I left it as ismake_mutone does areverse, which would be fine butLrc<[T]>does not implClone(should it?) - however, the reverse seems to be unnecessary - please check if the solution makes senseHad some unrelated-looking local test failures, let's see if if CI gets them too.