refactor: make Kind a lot smaller #82
Conversation
There was a problem hiding this comment.
It's funny cause it's almost the reverse commit of "Add more information in incompatibility::Kind " that I did back in September last year :)
My reasons were mainly that I found it made the code clearer, but I had no concern about memory at that time. I think both approaches can be justified. If you think it's worth it I trust you!
|
That is funny! The code clarity point is a valid one. Also, with this not helping with perf, I would be Ok with keeping this in our back pocket until we have a memory constrained user. I'm going to think about it some more before taking any action. |
|
I think most of the confusion comes from destructuring the This, something else, or leave if for now? Whatever you think. |
There was a problem hiding this comment.
I'm not a big fan of keeping only parts of the attributes to be honest. I think it may make the code confusing which is worst than having more code.
Also, somewhat related to this PR, I think we don't have property tests checking that a derivation tree is a valid proof of conflict. Saying this because I didn't check that the code building the derivation tree is valid.
| @@ -43,28 +43,25 @@ enum Kind<P: Package, V: Version> { | |||
| /// Initial incompatibility aiming at picking the root package for the first decision. | |||
| NotRoot(P, V), | |||
| /// There are no versions in the given range for this package. | |||
| NoVersions(P, Range<V>), | |||
| NoVersions(P), | |||
There was a problem hiding this comment.
I think things like NoVersion(P) might imply that there is no version at all for package P while actually that's limited to a given range.
| impl<K: PartialEq + Eq + Hash, V> Index<&K> for SmallMap<K, V> { | ||
| type Output = V; | ||
| fn index(&self, key: &K) -> &V { | ||
| &self.get(key).unwrap() |
There was a problem hiding this comment.
Reading terms[package] is nice but probably treacherous. I think it would be better if we could avoid this. Then the caller is aware of when they are calling unwrap().
| @@ -71,6 +71,14 @@ impl<V: Version> Term<V> { | |||
| _ => panic!("Negative term cannot unwrap positive range"), | |||
| } | |||
| } | |||
|
|
|||
| /// Unwrap the range contains in the term. | |||
| pub(crate) fn as_range(&self) -> &Range<V> { | |||
There was a problem hiding this comment.
I'm not a big fan of this function either. as_range on a term feels like it should be negated for a Negative(range). But a term can never be converted into a range without loosing on the semantics. So I'd probably go for a name such as unwrap() or similar, meaning that some information is lost. Maybe unwrap_range or extract_range.
That is a good point. |
|
In #157 (comment) reported large improvements from reducing the calls to |
This is two changes that make
Kinda lot smaller. That intern makesIncompatibilitysignificantly smaller. And as theincompatibility_storeis a large portion of memory usage, this reduces the total memory usage significantly.The trick is that data that is going into the
package_termsdoes not need to be kept in theKind.Using dhat-rs I am seeing the elm file go from:
to:
So a ~30% reduction.
Unfortunately that did not come with a speedup, but what can you do.