-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Use LazyBTreeMap for region constraints. #50491
Conversation
Because a lot of these BTreeMaps don't get anything inserted into them. In the most extreme case, this halves the number of total bytes allocated for a debug build of tuple-stress, from 2.2GB to 1.1GB. That reduction in turn speeds up runs of many benchmarks, the best by 3%.
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
Here are the benchmarks that had a run sped up by 1% or more.
|
@@ -124,7 +124,7 @@ pub fn maybe_print_constraints_for<'a, 'gcx, 'tcx>( | |||
struct ConstraintGraph<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { | |||
graph_name: String, | |||
region_rels: &'a RegionRelations<'a, 'gcx, 'tcx>, | |||
map: &'a BTreeMap<Constraint<'tcx>, SubregionOrigin<'tcx>>, | |||
map: &'a LazyBTreeMap<Constraint<'tcx>, SubregionOrigin<'tcx>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this even be an FxHashMap
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ConstraintGraph::map
is changed to an FxHashMap, then all the other LazyBTreeMap instances in this patch have to change to an FxHashMap as well. Maybe that's what you're suggesting? The various BTreeMaps are iterated over in multiple places. I don't know if the iteration order is important, but I'm inclined to be conservative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Constraint
just seems to have a derived Ord
implementation, so the order might not be important. I don't know this code though.
We should possibly close this on the basis that the proper BTreeMap fix is ready and just needs to be profiled? |
Fixed in a better way by #50352. |
Because a lot of these BTreeMaps don't get anything inserted into them.
In the most extreme case, this halves the number of total bytes
allocated for a debug build of tuple-stress, from 2.2GB to 1.1GB. That
reduction in turn speeds up runs of many benchmarks, the best by 3%.