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

Performance audit, Spring 2017 #41410

Closed
wants to merge 8 commits into from
Closed
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
6 changes: 2 additions & 4 deletions src/liballoc/heap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
issue = "27700")]

use core::{isize, usize};
#[cfg(not(test))]
use core::intrinsics::{min_align_of_val, size_of_val};

#[allow(improper_ctypes)]
Expand Down Expand Up @@ -158,10 +157,9 @@ unsafe fn exchange_malloc(size: usize, align: usize) -> *mut u8 {
}
}

#[cfg(not(test))]
#[lang = "box_free"]
#[cfg_attr(not(test), lang = "box_free")]
#[inline]
unsafe fn box_free<T: ?Sized>(ptr: *mut T) {
pub(crate) unsafe fn box_free<T: ?Sized>(ptr: *mut T) {
let size = size_of_val(&*ptr);
let align = min_align_of_val(&*ptr);
Copy link
Member

Choose a reason for hiding this comment

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

These two functions are not imported during testing (so this fails to compile then).

// We do not allocate for Box<T> when T is ZST, so deallocation is also not necessary.
Expand Down
1 change: 1 addition & 0 deletions src/liballoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
#![feature(needs_allocator)]
#![feature(optin_builtin_traits)]
#![feature(placement_in_syntax)]
#![cfg_attr(stage0, feature(pub_restricted))]
#![feature(shared)]
#![feature(staged_api)]
#![feature(unboxed_closures)]
Expand Down
35 changes: 33 additions & 2 deletions src/liballoc/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ use core::ops::CoerceUnsized;
use core::ptr::{self, Shared};
use core::convert::From;

use heap::deallocate;
use heap::{allocate, deallocate, box_free};
use raw_vec::RawVec;

struct RcBox<T: ?Sized> {
Expand All @@ -248,7 +248,6 @@ struct RcBox<T: ?Sized> {
value: T,
}


/// A single-threaded reference-counting pointer.
///
/// See the [module-level documentation](./index.html) for more details.
Expand Down Expand Up @@ -438,6 +437,38 @@ impl Rc<str> {
}
}

impl<T> Rc<[T]> {
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, for the purposes of the compiler, does Rc<[T]> provide a measurable improvement over Rc<Vec<T>>?

Copy link
Member

Choose a reason for hiding this comment

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

This may also be more easily implementable by consuming Vec<T> as you've got to copy data anyway. With Vec<T> the box_free also doesn't need to be exposed as you can just .set_len(0) to drop all the elements.

Copy link
Contributor Author

@arielb1 arielb1 Apr 20, 2017

Choose a reason for hiding this comment

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

Didn't bother checking. But Rc<Vec<T>> is too ugly to me.

/// Constructs a new `Rc<[T]>` from a `Box<[T]>`.
#[doc(hidden)]
#[unstable(feature = "rustc_private",
reason = "for internal use in rustc",
issue = "0")]
pub fn __from_array(value: Box<[T]>) -> Rc<[T]> {
unsafe {
let ptr: *mut RcBox<[T]> =
mem::transmute([mem::align_of::<RcBox<[T; 1]>>(), value.len()]);
// FIXME(custom-DST): creating this invalid &[T] is dubiously defined,
// we should have a better way of getting the size/align
// of a DST from its unsized part.
let ptr = allocate(size_of_val(&*ptr), align_of_val(&*ptr));
let ptr: *mut RcBox<[T]> = mem::transmute([ptr as usize, value.len()]);

// Initialize the new RcBox.
ptr::write(&mut (*ptr).strong, Cell::new(1));
ptr::write(&mut (*ptr).weak, Cell::new(1));
ptr::copy_nonoverlapping(
value.as_ptr(),
&mut (*ptr).value as *mut [T] as *mut T,
value.len());

// Free the original allocation without freeing its (moved) contents.
box_free(Box::into_raw(value));

Rc { ptr: Shared::new(ptr as *const _) }
}
}
}

impl<T: ?Sized> Rc<T> {
/// Creates a new [`Weak`][weak] pointer to this value.
///
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/infer/freshen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl<'a, 'gcx, 'tcx> TypeFolder<'gcx, 'tcx> for TypeFreshener<'a, 'gcx, 'tcx> {
ty::ReEmpty |
ty::ReErased => {
// replace all free regions with 'erased
self.tcx().mk_region(ty::ReErased)
self.tcx().types.re_erased
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/librustc/infer/region_inference/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,7 @@ impl<'a, 'gcx, 'tcx> RegionVarBindings<'a, 'gcx, 'tcx> {
} else {
// otherwise, we don't know what the free region is,
// so we must conservatively say the LUB is static:
self.tcx.mk_region(ReStatic)
self.tcx.types.re_static
}
}

Expand All @@ -971,7 +971,7 @@ impl<'a, 'gcx, 'tcx> RegionVarBindings<'a, 'gcx, 'tcx> {
if a == b {
a
} else {
self.tcx.mk_region(ReStatic)
self.tcx.types.re_static
}
}
}
Expand Down Expand Up @@ -1018,7 +1018,7 @@ impl<'a, 'gcx, 'tcx> RegionVarBindings<'a, 'gcx, 'tcx> {

fn construct_var_data(&self) -> Vec<VarValue<'tcx>> {
(0..self.num_vars() as usize)
.map(|_| Value(self.tcx.mk_region(ty::ReEmpty)))
.map(|_| Value(self.tcx.types.re_empty))
.collect()
}

Expand Down Expand Up @@ -1493,7 +1493,7 @@ fn lookup<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
-> &'tcx ty::Region {
match values[rid.index as usize] {
Value(r) => r,
ErrorValue => tcx.mk_region(ReStatic), // Previously reported error.
ErrorValue => tcx.types.re_static, // Previously reported error.
}
}

Expand Down
6 changes: 2 additions & 4 deletions src/librustc/middle/cstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,14 +188,13 @@ pub trait CrateStore {
fn visibility(&self, def: DefId) -> ty::Visibility;
fn visible_parent_map<'a>(&'a self) -> ::std::cell::Ref<'a, DefIdMap<DefId>>;
fn item_generics_cloned(&self, def: DefId) -> ty::Generics;
fn item_attrs(&self, def_id: DefId) -> Vec<ast::Attribute>;
fn item_attrs(&self, def_id: DefId) -> Rc<[ast::Attribute]>;
fn fn_arg_names(&self, did: DefId) -> Vec<ast::Name>;

// trait info
fn implementations_of_trait(&self, filter: Option<DefId>) -> Vec<DefId>;

// impl info
fn impl_polarity(&self, def: DefId) -> hir::ImplPolarity;
fn impl_parent(&self, impl_def_id: DefId) -> Option<DefId>;

// trait/impl-item info
Expand Down Expand Up @@ -323,14 +322,13 @@ impl CrateStore for DummyCrateStore {
}
fn item_generics_cloned(&self, def: DefId) -> ty::Generics
{ bug!("item_generics_cloned") }
fn item_attrs(&self, def_id: DefId) -> Vec<ast::Attribute> { bug!("item_attrs") }
fn item_attrs(&self, def_id: DefId) -> Rc<[ast::Attribute]> { bug!("item_attrs") }
fn fn_arg_names(&self, did: DefId) -> Vec<ast::Name> { bug!("fn_arg_names") }

// trait info
fn implementations_of_trait(&self, filter: Option<DefId>) -> Vec<DefId> { vec![] }

// impl info
fn impl_polarity(&self, def: DefId) -> hir::ImplPolarity { bug!("impl_polarity") }
fn impl_parent(&self, def: DefId) -> Option<DefId> { bug!("impl_parent") }

// trait/impl-item info
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/expr_use_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ impl<'a, 'gcx, 'tcx> ExprUseVisitor<'a, 'gcx, 'tcx> {

hir::ExprMatch(ref discr, ref arms, _) => {
let discr_cmt = return_if_err!(self.mc.cat_expr(&discr));
let r = self.tcx().mk_region(ty::ReEmpty);
let r = self.tcx().types.re_empty;
self.borrow_expr(&discr, r, ty::ImmBorrow, MatchDiscriminant);

// treatment of the discriminant is handled while walking the arms.
Expand Down
7 changes: 4 additions & 3 deletions src/librustc/middle/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,10 @@ impl<'a, 'tcx> LanguageItemCollector<'a, 'tcx> {

pub fn extract(attrs: &[ast::Attribute]) -> Option<Symbol> {
for attribute in attrs {
match attribute.value_str() {
Some(value) if attribute.check_name("lang") => return Some(value),
_ => {}
if attribute.check_name("lang") {
if let Some(value) = attribute.value_str() {
return Some(value)
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/librustc/middle/mem_categorization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -871,8 +871,8 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> {
// we can promote to a constant, otherwise equal to enclosing temp
// lifetime.
let (re, old_re) = if promotable {
(self.tcx().mk_region(ty::ReStatic),
self.tcx().mk_region(ty::ReStatic))
(self.tcx().types.re_static,
self.tcx().types.re_static)
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that we don't have these constants pre-interned. This is going to conflict like hell with one of my in-progress branches, but oh well. =)

} else {
self.temporary_scope(id)
};
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ fn process_predicate<'a, 'gcx, 'tcx>(
// Otherwise, we have something of the form
// `for<'a> T: 'a where 'a not in T`, which we can treat as `T: 'static`.
Some(t_a) => {
let r_static = selcx.tcx().mk_region(ty::ReStatic);
let r_static = selcx.tcx().types.re_static;
register_region_obligation(t_a, r_static,
obligation.cause.clone(),
region_obligations);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ pub fn get_vtable_methods<'a, 'tcx>(
// the method may have some early-bound lifetimes, add
// regions for those
let substs = Substs::for_item(tcx, def_id,
|_, _| tcx.mk_region(ty::ReErased),
|_, _| tcx.types.re_erased,
|def, _| trait_ref.substs().type_for_def(def));

// the trait type may have higher-ranked lifetimes in it;
Expand Down
14 changes: 7 additions & 7 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -943,17 +943,17 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> {
debug!("Retaining candidate #{}/{}: {:?}",
i, candidates.len(), candidates[i]);
i += 1;

// If there are *STILL* multiple candidates, give up
// and report ambiguity.
if i > 1 {
debug!("multiple matches, ambig");
return Ok(None);
}
}
}
}

// If there are *STILL* multiple candidates, give up and
// report ambiguity.
if candidates.len() > 1 {
debug!("multiple matches, ambig");
return Ok(None);
}

// If there are *NO* candidates, then there are no impls --
// that we know of, anyway. Note that in the case where there
// are unbound type variables within the obligation, it might
Expand Down
18 changes: 17 additions & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,10 @@ pub struct CommonTypes<'tcx> {
pub f64: Ty<'tcx>,
pub never: Ty<'tcx>,
pub err: Ty<'tcx>,

pub re_empty: &'tcx Region,
pub re_static: &'tcx Region,
pub re_erased: &'tcx Region,
}

#[derive(RustcEncodable, RustcDecodable)]
Expand Down Expand Up @@ -360,6 +364,14 @@ impl<'tcx> TypeckTables<'tcx> {
impl<'tcx> CommonTypes<'tcx> {
fn new(interners: &CtxtInterners<'tcx>) -> CommonTypes<'tcx> {
let mk = |sty| interners.intern_ty(sty, None);
let mk_region = |r| {
if let Some(r) = interners.region.borrow().get(&r) {
return r.0;
}
let r = interners.arena.alloc(r);
interners.region.borrow_mut().insert(Interned(r));
&*r
};
CommonTypes {
bool: mk(TyBool),
char: mk(TyChar),
Expand All @@ -379,6 +391,10 @@ impl<'tcx> CommonTypes<'tcx> {
u128: mk(TyUint(ast::UintTy::U128)),
f32: mk(TyFloat(ast::FloatTy::F32)),
f64: mk(TyFloat(ast::FloatTy::F64)),

re_empty: mk_region(Region::ReEmpty),
re_static: mk_region(Region::ReStatic),
re_erased: mk_region(Region::ReErased),
}
}
}
Expand Down Expand Up @@ -1233,7 +1249,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
}

pub fn mk_static_str(self) -> Ty<'tcx> {
self.mk_imm_ref(self.mk_region(ty::ReStatic), self.mk_str())
self.mk_imm_ref(self.types.re_static, self.mk_str())
}

pub fn mk_adt(self, def: &'tcx AdtDef, substs: &'tcx Substs<'tcx>) -> Ty<'tcx> {
Expand Down
22 changes: 19 additions & 3 deletions src/librustc/ty/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub fn erase_late_bound_regions<T>(self, value: &Binder<T>) -> T
where T : TypeFoldable<'tcx>
{
self.replace_late_bound_regions(value, |_| self.mk_region(ty::ReErased)).0
self.replace_late_bound_regions(value, |_| self.types.re_erased).0
}

/// Rewrite any late-bound regions so that they are anonymous. Region numbers are
Expand Down Expand Up @@ -538,7 +538,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
// whenever a substitution occurs.
match *r {
ty::ReLateBound(..) => r,
_ => self.tcx().mk_region(ty::ReErased)
_ => self.tcx().types.re_erased
}
}
}
Expand All @@ -565,6 +565,22 @@ pub fn shift_region(region: ty::Region, amount: u32) -> ty::Region {
}
}

pub fn shift_region_ref<'a, 'gcx, 'tcx>(
tcx: TyCtxt<'a, 'gcx, 'tcx>,
region: &'tcx ty::Region,
amount: u32)
-> &'tcx ty::Region
{
match region {
&ty::ReLateBound(debruijn, br) if amount > 0 => {
tcx.mk_region(ty::ReLateBound(debruijn.shifted(amount), br))
}
_ => {
region
}
}
}

pub fn shift_regions<'a, 'gcx, 'tcx, T>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
amount: u32, value: &T) -> T
where T: TypeFoldable<'tcx>
Expand All @@ -573,7 +589,7 @@ pub fn shift_regions<'a, 'gcx, 'tcx, T>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
value, amount);

value.fold_with(&mut RegionFolder::new(tcx, &mut false, &mut |region, _current_depth| {
tcx.mk_region(shift_region(*region, amount))
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow. By-value shift_region may have been used by elision or something. Can we just kill it?

shift_region_ref(tcx, region, amount)
}))
}

Expand Down
5 changes: 1 addition & 4 deletions src/librustc/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@ use hir::def_id::DefId;
use ty::{self, Ty, TypeFoldable, Substs};
use util::ppaux;

use std::borrow::Cow;
use std::fmt;
use syntax::ast;


#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
pub struct Instance<'tcx> {
Expand Down Expand Up @@ -59,7 +56,7 @@ impl<'tcx> InstanceDef<'tcx> {
}

#[inline]
pub fn attrs<'a>(&self, tcx: ty::TyCtxt<'a, 'tcx, 'tcx>) -> Cow<'tcx, [ast::Attribute]> {
pub fn attrs<'a>(&self, tcx: ty::TyCtxt<'a, 'tcx, 'tcx>) -> ty::Attributes<'tcx> {
tcx.get_attrs(self.def_id())
}

Expand Down
Loading