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.

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

Speed up NLL with HybridIdxSetBuf. #53383

Merged
merged 1 commit into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
247 changes: 237 additions & 10 deletions src/librustc_data_structures/indexed_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use array_vec::ArrayVec;
use std::borrow::{Borrow, BorrowMut, ToOwned};
use std::fmt;
use std::iter;
Expand All @@ -25,6 +26,8 @@ use rustc_serialize;
///
/// In other words, `T` is the type used to index into the bitvector
/// this type uses to represent the set of object it holds.
///
/// The representation is dense, using one bit per possible element.
#[derive(Eq, PartialEq)]
pub struct IdxSetBuf<T: Idx> {
_pd: PhantomData<fn(&T)>,
Expand Down Expand Up @@ -93,6 +96,8 @@ impl<T: Idx> ToOwned for IdxSet<T> {
}
}

const BITS_PER_WORD: usize = mem::size_of::<Word>() * 8;

impl<T: Idx> fmt::Debug for IdxSetBuf<T> {
fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
w.debug_list()
Expand All @@ -111,8 +116,7 @@ impl<T: Idx> fmt::Debug for IdxSet<T> {

impl<T: Idx> IdxSetBuf<T> {
fn new(init: Word, universe_size: usize) -> Self {
let bits_per_word = mem::size_of::<Word>() * 8;
let num_words = (universe_size + (bits_per_word - 1)) / bits_per_word;
let num_words = (universe_size + (BITS_PER_WORD - 1)) / BITS_PER_WORD;
IdxSetBuf {
_pd: Default::default(),
bits: vec![init; num_words],
Expand Down Expand Up @@ -163,6 +167,16 @@ impl<T: Idx> IdxSet<T> {
}
}

/// Duplicates as a hybrid set.
pub fn to_hybrid(&self) -> HybridIdxSetBuf<T> {
// This universe_size may be slightly larger than the one specified
// upon creation, due to rounding up to a whole word. That's ok.
let universe_size = self.bits.len() * BITS_PER_WORD;

// Note: we currently don't bother trying to make a Sparse set.
HybridIdxSetBuf::Dense(self.to_owned(), universe_size)
}

/// Removes all elements
pub fn clear(&mut self) {
for b in &mut self.bits {
Expand All @@ -180,21 +194,19 @@ impl<T: Idx> IdxSet<T> {

/// Clear all elements above `universe_size`.
fn trim_to(&mut self, universe_size: usize) {
let word_bits = mem::size_of::<Word>() * 8;

// `trim_block` is the first block where some bits have
// to be cleared.
let trim_block = universe_size / word_bits;
let trim_block = universe_size / BITS_PER_WORD;

// all the blocks above it have to be completely cleared.
if trim_block < self.bits.len() {
for b in &mut self.bits[trim_block+1..] {
*b = 0;
}

// at that block, the `universe_size % word_bits` lsbs
// at that block, the `universe_size % BITS_PER_WORD` lsbs
// should remain.
let remaining_bits = universe_size % word_bits;
let remaining_bits = universe_size % BITS_PER_WORD;
let mask = (1<<remaining_bits)-1;
self.bits[trim_block] &= mask;
}
Expand Down Expand Up @@ -245,12 +257,46 @@ impl<T: Idx> IdxSet<T> {
bitwise(self.words_mut(), other.words(), &Union)
}

/// Like `union()`, but takes a `SparseIdxSetBuf` argument.
fn union_sparse(&mut self, other: &SparseIdxSetBuf<T>) -> bool {
let mut changed = false;
for elem in other.iter() {
changed |= self.add(&elem);
}
changed
}

/// Like `union()`, but takes a `HybridIdxSetBuf` argument.
pub fn union_hybrid(&mut self, other: &HybridIdxSetBuf<T>) -> bool {
match other {
HybridIdxSetBuf::Sparse(sparse, _) => self.union_sparse(sparse),
HybridIdxSetBuf::Dense(dense, _) => self.union(dense),
}
}

/// Set `self = self - other` and return true if `self` changed.
/// (i.e., if any bits were removed).
pub fn subtract(&mut self, other: &IdxSet<T>) -> bool {
bitwise(self.words_mut(), other.words(), &Subtract)
}

/// Like `subtract()`, but takes a `SparseIdxSetBuf` argument.
fn subtract_sparse(&mut self, other: &SparseIdxSetBuf<T>) -> bool {
let mut changed = false;
for elem in other.iter() {
changed |= self.remove(&elem);
}
changed
}

/// Like `subtract()`, but takes a `HybridIdxSetBuf` argument.
pub fn subtract_hybrid(&mut self, other: &HybridIdxSetBuf<T>) -> bool {
match other {
HybridIdxSetBuf::Sparse(sparse, _) => self.subtract_sparse(sparse),
HybridIdxSetBuf::Dense(dense, _) => self.subtract(dense),
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it's a good idea to have a trait instead of different methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the trait look like? I'm having trouble imagining it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A trait would read nicer, for sure, but we could leave it to follow-up. I imagine it would be something like:

trait SubtractFrom<T> {
    fn subtract_from(&self, target: &mut IdxSetBuf<T>) -> bool;
}

and then you would have

impl<T> IdxSetBuf<T> {
    fn subtract(&mut self, other: &impl SubtractFrom<T>) -> bool {
        other.subtract_from(self)
    }
}

and then

impl<T> SubtractFrom<T> for IdxSetBuf<T> { .. }
impl<T> SubtractFrom<T> for HybridIdxSetBuf<T> { .. }
impl<T> SubtractFrom<T> for SparseIdxSetBuf<T> { .. }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave as-is for now and maybe clean it up a little later. I'm not actually sure that the trait would be a win for readability anyway.


/// Set `self = self & other` and return true if `self` changed.
/// (i.e., if any bits were removed).
pub fn intersect(&mut self, other: &IdxSet<T>) -> bool {
Expand All @@ -276,19 +322,200 @@ impl<'a, T: Idx> Iterator for Iter<'a, T> {
type Item = T;

fn next(&mut self) -> Option<T> {
let word_bits = mem::size_of::<Word>() * 8;
loop {
if let Some((ref mut word, offset)) = self.cur {
let bit_pos = word.trailing_zeros() as usize;
if bit_pos != word_bits {
if bit_pos != BITS_PER_WORD {
let bit = 1 << bit_pos;
*word ^= bit;
return Some(T::new(bit_pos + offset))
}
}

let (i, word) = self.iter.next()?;
self.cur = Some((*word, word_bits * i));
self.cur = Some((*word, BITS_PER_WORD * i));
}
}
}

const SPARSE_MAX: usize = 8;

/// A sparse index set with a maximum of SPARSE_MAX elements. Used by
/// HybridIdxSetBuf; do not use directly.
///
/// The elements are stored as an unsorted vector with no duplicates.
#[derive(Clone, Debug)]
pub struct SparseIdxSetBuf<T: Idx>(ArrayVec<[T; SPARSE_MAX]>);

impl<T: Idx> SparseIdxSetBuf<T> {
fn new() -> Self {
SparseIdxSetBuf(ArrayVec::new())
}

fn len(&self) -> usize {
self.0.len()
}

fn contains(&self, elem: &T) -> bool {
self.0.contains(elem)
}

fn add(&mut self, elem: &T) -> bool {
// Ensure there are no duplicates.
if self.0.contains(elem) {
false
} else {
self.0.push(*elem);
true
}
}

fn remove(&mut self, elem: &T) -> bool {
if let Some(i) = self.0.iter().position(|e| e == elem) {
// Swap the found element to the end, then pop it.
let len = self.0.len();
self.0.swap(i, len - 1);
self.0.pop();
true
} else {
false
}
}

fn to_dense(&self, universe_size: usize) -> IdxSetBuf<T> {
let mut dense = IdxSetBuf::new_empty(universe_size);
for elem in self.0.iter() {
dense.add(elem);
}
dense
}

fn iter(&self) -> SparseIter<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were going to use impl trait, this could just be

fn iter(&self) -> impl Iterator<Item = T> + '_ {
    self.iter().cloned()
}

SparseIter {
iter: self.0.iter(),
}
}
}

pub struct SparseIter<'a, T: Idx> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably instead use impl Trait and pass through the various iterator traits; DoubleEnded, TrustedLen, etc. Otherwise we're losing performance here...

We should at least be able to implement size_hint, count, and nth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point at some existing code that does similar things? I'm not sure what this would look like. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, FWIW, I just copied the code in the existing Iter for IdxSetBuf. So if there is a genuine win to be had here -- and profiling doesn't indicate so, at least for the moment -- it should be changed as well. Perhaps that could also be a follow-up.

iter: slice::Iter<'a, T>,
}

impl<'a, T: Idx> Iterator for SparseIter<'a, T> {
type Item = T;

fn next(&mut self) -> Option<T> {
self.iter.next().map(|e| *e)
}
}

/// Like IdxSetBuf, but with a hybrid representation: sparse when there are few
/// elements in the set, but dense when there are many. It's especially
/// efficient for sets that typically have a small number of elements, but a
/// large `universe_size`, and are cleared frequently.
#[derive(Clone, Debug)]
pub enum HybridIdxSetBuf<T: Idx> {
Sparse(SparseIdxSetBuf<T>, usize),
Dense(IdxSetBuf<T>, usize),
}

impl<T: Idx> HybridIdxSetBuf<T> {
pub fn new_empty(universe_size: usize) -> Self {
HybridIdxSetBuf::Sparse(SparseIdxSetBuf::new(), universe_size)
}

fn universe_size(&mut self) -> usize {
match *self {
HybridIdxSetBuf::Sparse(_, size) => size,
HybridIdxSetBuf::Dense(_, size) => size,
}
}

pub fn clear(&mut self) {
let universe_size = self.universe_size();
*self = HybridIdxSetBuf::new_empty(universe_size);
}

/// Returns true iff set `self` contains `elem`.
pub fn contains(&self, elem: &T) -> bool {
match self {
HybridIdxSetBuf::Sparse(sparse, _) => sparse.contains(elem),
HybridIdxSetBuf::Dense(dense, _) => dense.contains(elem),
}
}

/// Adds `elem` to the set `self`.
pub fn add(&mut self, elem: &T) -> bool {
match self {
HybridIdxSetBuf::Sparse(sparse, _) if sparse.len() < SPARSE_MAX => {
// The set is sparse and has space for `elem`.
sparse.add(elem)
}
HybridIdxSetBuf::Sparse(sparse, _) if sparse.contains(elem) => {
// The set is sparse and does not have space for `elem`, but
// that doesn't matter because `elem` is already present.
false
}
HybridIdxSetBuf::Sparse(_, _) => {
// The set is sparse and full. Convert to a dense set.
//
// FIXME: This code is awful, but I can't work out how else to
// appease the borrow checker.
let dummy = HybridIdxSetBuf::Sparse(SparseIdxSetBuf::new(), 0);
match mem::replace(self, dummy) {
HybridIdxSetBuf::Sparse(sparse, universe_size) => {
let mut dense = sparse.to_dense(universe_size);
let changed = dense.add(elem);
assert!(changed);
mem::replace(self, HybridIdxSetBuf::Dense(dense, universe_size));
changed
}
_ => panic!("impossible"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an unreachable! I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but ideally someone would point out a way to rewrite the code so this isn't necessary :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if let HybridIdxSetBuf::Sparse(sparse, universe_size) = mem::replace(self, dummy)? It doesn't require an else branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I find that misleading -- it's strange to have an if let that cannot fail. The match makes the cannot-fail-ness more obvious.

}
}

HybridIdxSetBuf::Dense(dense, _) => dense.add(elem),
}
}

/// Removes `elem` from the set `self`.
pub fn remove(&mut self, elem: &T) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to take by reference here since I'd is Copy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied the existing method in IdxSetBuf, so I will leave this alone. Converting all such methods in this file could be done as a follow-up.

// Note: we currently don't bother going from Dense back to Sparse.
match self {
HybridIdxSetBuf::Sparse(sparse, _) => sparse.remove(elem),
HybridIdxSetBuf::Dense(dense, _) => dense.remove(elem),
}
}

/// Converts to a dense set, consuming itself in the process.
pub fn to_dense(self) -> IdxSetBuf<T> {
match self {
HybridIdxSetBuf::Sparse(sparse, universe_size) => sparse.to_dense(universe_size),
HybridIdxSetBuf::Dense(dense, _) => dense,
}
}

/// Iteration order is unspecified.
pub fn iter(&self) -> HybridIter<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here again you could use impl Trait, though you have to pick from two different iterator types so it is a bit more annoying. There would be two options: to use the Either type or to use a single chained type. I tend to prefer the latter.

pub fn iter(&self) -> impl Iterator<Item = T> {
    let (sparse_iter, dense_iter) = match self {
        HybridIdxSetBuf::Sparse(sparse, _) => (Some(sparse.iter()), None),
        HybridIdxSetBuf::Dense(dense, _) => (None, Some(dense.iter()),
    }
    sparse_iter
        .into_iter()
        .flatten()
        .chain(dense_iter.into_iter().flatten())
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this because it ought to be mildly more efficient, since you kind of pick which iterator you will use just once, in the beginning, vs on every element. But your current setup is closer to EIther, which is more obvious. It would look like:

use either::Either;
pub fn iter(&self) -> impl Iterator<Item = T> {
    match self {
        HybridIdxSetBuf::Sparse(sparse, _) => Either::Left(sparse.iter()),
        HybridIdxSetBuf::Dense(dense, _) => Either::Right(dense.iter()),
    }
}

that assumes that rustc_data_structures already takes the either crate as a dependency, though. I know that some of our crates do.

match self {
HybridIdxSetBuf::Sparse(sparse, _) => HybridIter::Sparse(sparse.iter()),
HybridIdxSetBuf::Dense(dense, _) => HybridIter::Dense(dense.iter()),
}
}
}

pub enum HybridIter<'a, T: Idx> {
Sparse(SparseIter<'a, T>),
Dense(Iter<'a, T>),
}

impl<'a, T: Idx> Iterator for HybridIter<'a, T> {
type Item = T;

fn next(&mut self) -> Option<T> {
match self {
HybridIter::Sparse(sparse) => sparse.next(),
HybridIter::Dense(dense) => dense.next(),
}
}
}
Expand Down
18 changes: 9 additions & 9 deletions src/librustc_mir/dataflow/at_location.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
//! locations.

use rustc::mir::{BasicBlock, Location};
use rustc_data_structures::indexed_set::{IdxSetBuf, Iter};
use rustc_data_structures::indexed_set::{HybridIdxSetBuf, IdxSetBuf, Iter};
use rustc_data_structures::indexed_vec::Idx;

use dataflow::{BitDenotation, BlockSets, DataflowResults};
Expand Down Expand Up @@ -68,8 +68,8 @@ where
{
base_results: DataflowResults<BD>,
curr_state: IdxSetBuf<BD::Idx>,
stmt_gen: IdxSetBuf<BD::Idx>,
stmt_kill: IdxSetBuf<BD::Idx>,
stmt_gen: HybridIdxSetBuf<BD::Idx>,
stmt_kill: HybridIdxSetBuf<BD::Idx>,
}

impl<BD> FlowAtLocation<BD>
Expand Down Expand Up @@ -97,8 +97,8 @@ where
pub fn new(results: DataflowResults<BD>) -> Self {
let bits_per_block = results.sets().bits_per_block();
let curr_state = IdxSetBuf::new_empty(bits_per_block);
let stmt_gen = IdxSetBuf::new_empty(bits_per_block);
let stmt_kill = IdxSetBuf::new_empty(bits_per_block);
let stmt_gen = HybridIdxSetBuf::new_empty(bits_per_block);
let stmt_kill = HybridIdxSetBuf::new_empty(bits_per_block);
FlowAtLocation {
base_results: results,
curr_state: curr_state,
Expand Down Expand Up @@ -129,8 +129,8 @@ where
F: FnOnce(Iter<BD::Idx>),
{
let mut curr_state = self.curr_state.clone();
curr_state.union(&self.stmt_gen);
curr_state.subtract(&self.stmt_kill);
curr_state.union_hybrid(&self.stmt_gen);
curr_state.subtract_hybrid(&self.stmt_kill);
f(curr_state.iter());
}
}
Expand Down Expand Up @@ -193,8 +193,8 @@ impl<BD> FlowsAtLocation for FlowAtLocation<BD>
}

fn apply_local_effect(&mut self, _loc: Location) {
self.curr_state.union(&self.stmt_gen);
self.curr_state.subtract(&self.stmt_kill);
self.curr_state.union_hybrid(&self.stmt_gen);
self.curr_state.subtract_hybrid(&self.stmt_kill);
}
}

Expand Down
Loading