Skip to content

Commit

Permalink
Auto merge of #50240 - nnethercote:LazyBTreeMap, r=cramertj
Browse files Browse the repository at this point in the history
Implement LazyBTreeMap and use it in a few places.

This is a thin wrapper around BTreeMap that avoids allocating upon creation.

I would prefer to change BTreeMap directly to make it lazy (like I did with HashSet in #36734) and I initially attempted that by making BTreeMap::root an Option<>. But then I also had to change Iter and Range to handle trees with no root, and those types have stability markers on them and I wasn't sure if that was acceptable. Also, BTreeMap has a lot of complex code and changing it all was challenging, and I didn't have high confidence about my general approach.

So I prototyped this wrapper instead and used it in the hottest locations to get some measurements about the effect. The measurements are pretty good!

- Doing a debug build of serde, it reduces the total number of heap allocations from 17,728,709 to 13,359,384, a 25% reduction. The number of bytes allocated drops from 7,474,672,966 to 5,482,308,388, a 27% reduction.

- It gives speedups of up to 3.6% on some rustc-perf benchmark jobs. crates.io, futures, and serde benefit most.
```
futures-check
        avg: -1.9%      min: -3.6%      max: -0.5%
serde-check
        avg: -2.1%      min: -3.5%      max: -0.7%
crates.io-check
        avg: -1.7%      min: -3.5%      max: -0.3%
serde
        avg: -2.0%      min: -3.0%      max: -0.9%
serde-opt
        avg: -1.8%      min: -2.9%      max: -0.3%
futures
        avg: -1.5%      min: -2.8%      max: -0.4%
tokio-webpush-simple-check
        avg: -1.1%      min: -2.2%      max: -0.1%
futures-opt
        avg: -1.2%      min: -2.1%      max: -0.4%
piston-image-check
        avg: -0.8%      min: -1.1%      max: -0.3%
crates.io
        avg: -0.6%      min: -1.0%      max: -0.3%
```
@gankro, how do you think I should proceed here? Is leaving this as a wrapper reasonable? Or should I try to make BTreeMap itself lazy? If so, can I change the representation of Iter and Range?

Thanks!
  • Loading branch information
bors committed Apr 28, 2018
2 parents 68a09fc + 259ae18 commit 66363b2
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 12 deletions.
12 changes: 7 additions & 5 deletions src/librustc/infer/higher_ranked/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use super::{CombinedSnapshot,
use super::combine::CombineFields;
use super::region_constraints::{TaintDirections};

use std::collections::BTreeMap;
use rustc_data_structures::lazy_btree_map::LazyBTreeMap;
use ty::{self, TyCtxt, Binder, TypeFoldable};
use ty::error::TypeError;
use ty::relate::{Relate, RelateResult, TypeRelation};
Expand Down Expand Up @@ -247,7 +247,8 @@ impl<'a, 'gcx, 'tcx> CombineFields<'a, 'gcx, 'tcx> {
snapshot: &CombinedSnapshot<'a, 'tcx>,
debruijn: ty::DebruijnIndex,
new_vars: &[ty::RegionVid],
a_map: &BTreeMap<ty::BoundRegion, ty::Region<'tcx>>,
a_map: &LazyBTreeMap<ty::BoundRegion,
ty::Region<'tcx>>,
r0: ty::Region<'tcx>)
-> ty::Region<'tcx> {
// Regions that pre-dated the LUB computation stay as they are.
Expand Down Expand Up @@ -343,7 +344,8 @@ impl<'a, 'gcx, 'tcx> CombineFields<'a, 'gcx, 'tcx> {
snapshot: &CombinedSnapshot<'a, 'tcx>,
debruijn: ty::DebruijnIndex,
new_vars: &[ty::RegionVid],
a_map: &BTreeMap<ty::BoundRegion, ty::Region<'tcx>>,
a_map: &LazyBTreeMap<ty::BoundRegion,
ty::Region<'tcx>>,
a_vars: &[ty::RegionVid],
b_vars: &[ty::RegionVid],
r0: ty::Region<'tcx>)
Expand Down Expand Up @@ -412,7 +414,7 @@ impl<'a, 'gcx, 'tcx> CombineFields<'a, 'gcx, 'tcx> {

fn rev_lookup<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>,
span: Span,
a_map: &BTreeMap<ty::BoundRegion, ty::Region<'tcx>>,
a_map: &LazyBTreeMap<ty::BoundRegion, ty::Region<'tcx>>,
r: ty::Region<'tcx>) -> ty::Region<'tcx>
{
for (a_br, a_r) in a_map {
Expand All @@ -435,7 +437,7 @@ impl<'a, 'gcx, 'tcx> CombineFields<'a, 'gcx, 'tcx> {
}

fn var_ids<'a, 'gcx, 'tcx>(fields: &CombineFields<'a, 'gcx, 'tcx>,
map: &BTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
map: &LazyBTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
-> Vec<ty::RegionVid> {
map.iter()
.map(|(_, &r)| match *r {
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ use ty::error::{ExpectedFound, TypeError, UnconstrainedNumeric};
use ty::fold::TypeFoldable;
use ty::relate::RelateResult;
use traits::{self, ObligationCause, PredicateObligations};
use rustc_data_structures::lazy_btree_map::LazyBTreeMap;
use rustc_data_structures::unify as ut;
use std::cell::{Cell, RefCell, Ref, RefMut};
use std::collections::BTreeMap;
use std::fmt;
use syntax::ast;
use errors::DiagnosticBuilder;
Expand Down Expand Up @@ -187,7 +187,7 @@ pub struct InferCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {

/// A map returned by `skolemize_late_bound_regions()` indicating the skolemized
/// region that each late-bound region was replaced with.
pub type SkolemizationMap<'tcx> = BTreeMap<ty::BoundRegion, ty::Region<'tcx>>;
pub type SkolemizationMap<'tcx> = LazyBTreeMap<ty::BoundRegion, ty::Region<'tcx>>;

/// See `error_reporting` module for more details
#[derive(Clone, Debug)]
Expand Down Expand Up @@ -1216,7 +1216,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
span: Span,
lbrct: LateBoundRegionConversionTime,
value: &ty::Binder<T>)
-> (T, BTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
-> (T, LazyBTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
where T : TypeFoldable<'tcx>
{
self.tcx.replace_late_bound_regions(
Expand Down
8 changes: 4 additions & 4 deletions src/librustc/ty/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ use middle::const_val::ConstVal;
use hir::def_id::DefId;
use ty::{self, Binder, Ty, TyCtxt, TypeFlags};

use rustc_data_structures::lazy_btree_map::LazyBTreeMap;
use std::fmt;
use std::collections::BTreeMap;
use util::nodemap::FxHashSet;

/// The TypeFoldable trait is implemented for every type that can be folded.
Expand Down Expand Up @@ -334,7 +334,7 @@ struct RegionReplacer<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
tcx: TyCtxt<'a, 'gcx, 'tcx>,
current_depth: u32,
fld_r: &'a mut (dyn FnMut(ty::BoundRegion) -> ty::Region<'tcx> + 'a),
map: BTreeMap<ty::BoundRegion, ty::Region<'tcx>>
map: LazyBTreeMap<ty::BoundRegion, ty::Region<'tcx>>
}

impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
Expand All @@ -349,7 +349,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub fn replace_late_bound_regions<T,F>(self,
value: &Binder<T>,
mut f: F)
-> (T, BTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
-> (T, LazyBTreeMap<ty::BoundRegion, ty::Region<'tcx>>)
where F : FnMut(ty::BoundRegion) -> ty::Region<'tcx>,
T : TypeFoldable<'tcx>,
{
Expand Down Expand Up @@ -462,7 +462,7 @@ impl<'a, 'gcx, 'tcx> RegionReplacer<'a, 'gcx, 'tcx> {
tcx,
current_depth: 1,
fld_r,
map: BTreeMap::default()
map: LazyBTreeMap::default()
}
}
}
Expand Down
108 changes: 108 additions & 0 deletions src/librustc_data_structures/lazy_btree_map.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::collections::btree_map;
use std::collections::BTreeMap;

/// A thin wrapper around BTreeMap that avoids allocating upon creation.
///
/// Vec, HashSet and HashMap all have the nice feature that they don't do any
/// heap allocation when creating a new structure of the default size. In
/// contrast, BTreeMap *does* allocate in that situation. The compiler uses
/// B-Tree maps in some places such that many maps are created but few are
/// inserted into, so having a BTreeMap alternative that avoids allocating on
/// creation is a performance win.
///
/// Only a fraction of BTreeMap's functionality is currently supported.
/// Additional functionality should be added on demand.
#[derive(Debug)]
pub struct LazyBTreeMap<K, V>(Option<BTreeMap<K, V>>);

impl<K, V> LazyBTreeMap<K, V> {
pub fn new() -> LazyBTreeMap<K, V> {
LazyBTreeMap(None)
}

pub fn iter(&self) -> Iter<K, V> {
Iter(self.0.as_ref().map(|btm| btm.iter()))
}

pub fn is_empty(&self) -> bool {
self.0.as_ref().map_or(true, |btm| btm.is_empty())
}
}

impl<K: Ord, V> LazyBTreeMap<K, V> {
fn instantiate(&mut self) -> &mut BTreeMap<K, V> {
if let Some(ref mut btm) = self.0 {
btm
} else {
let btm = BTreeMap::new();
self.0 = Some(btm);
self.0.as_mut().unwrap()
}
}

pub fn insert(&mut self, key: K, value: V) -> Option<V> {
self.instantiate().insert(key, value)
}

pub fn entry(&mut self, key: K) -> btree_map::Entry<K, V> {
self.instantiate().entry(key)
}

pub fn values<'a>(&'a self) -> Values<'a, K, V> {
Values(self.0.as_ref().map(|btm| btm.values()))
}
}

impl<K: Ord, V> Default for LazyBTreeMap<K, V> {
fn default() -> LazyBTreeMap<K, V> {
LazyBTreeMap::new()
}
}

impl<'a, K: 'a, V: 'a> IntoIterator for &'a LazyBTreeMap<K, V> {
type Item = (&'a K, &'a V);
type IntoIter = Iter<'a, K, V>;

fn into_iter(self) -> Iter<'a, K, V> {
self.iter()
}
}

pub struct Iter<'a, K: 'a, V: 'a>(Option<btree_map::Iter<'a, K, V>>);

impl<'a, K: 'a, V: 'a> Iterator for Iter<'a, K, V> {
type Item = (&'a K, &'a V);

fn next(&mut self) -> Option<(&'a K, &'a V)> {
self.0.as_mut().and_then(|iter| iter.next())
}

fn size_hint(&self) -> (usize, Option<usize>) {
self.0.as_ref().map_or_else(|| (0, Some(0)), |iter| iter.size_hint())
}
}

pub struct Values<'a, K: 'a, V: 'a>(Option<btree_map::Values<'a, K, V>>);

impl<'a, K, V> Iterator for Values<'a, K, V> {
type Item = &'a V;

fn next(&mut self) -> Option<&'a V> {
self.0.as_mut().and_then(|values| values.next())
}

fn size_hint(&self) -> (usize, Option<usize>) {
self.0.as_ref().map_or_else(|| (0, Some(0)), |values| values.size_hint())
}
}

1 change: 1 addition & 0 deletions src/librustc_data_structures/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ pub mod bitvec;
pub mod graph;
pub mod indexed_set;
pub mod indexed_vec;
pub mod lazy_btree_map;
pub mod obligation_forest;
pub mod sip128;
pub mod snapshot_map;
Expand Down

0 comments on commit 66363b2

Please sign in to comment.