perf: Use SmallVec in the Type::App variant #129

Marwes · 2016-08-24T19:50:51Z

Since applications are usually done on just a few argument it is usually unnecessary to allocate a Vec. Using SmallVec this heap allocation can be avoided for applications with only a few arguments which should cover most of the cases.
#50

Marwes · 2016-08-24T19:59:38Z

Not as happy as I expected with this PR as SmallVec currently has two drop flags which causes Type to grow by 8 bytes. Once rust-lang/rust#35764 gets merged it should be possible to actually increase the number of arguments to 3 (from the current 2) while still keeping the same size for Type (thought naturally this would be worse for anything else than nightly until the trains catch up).

Atm I am only seeing a minor improvement in the typecheck_prelude benchmark so maybe hold of on merging this until it can be tested with the aforementioned rustc PR.

lambda-fairy · 2016-10-03T09:25:23Z

Heads up, rust-lang/rust#35764 was merged a while ago. Might like to revisit this change.

Marwes · 2016-10-03T10:13:51Z

I think I redid the benchmarks after rust-lang/rust#35764 was merged but didn't see any significant improvements (allocation does not seem to be a large part of the runtime currently, but admittedly it might be shadowed by other things). Going to redo it anyway to verify (and remember to update this PR).

I believe drop-flags will only be removed when 1.13 is released however so In any case it might be best waiting for 1.13 before merging.

Marwes · 2016-10-04T16:45:09Z

Rebased and removed the intermediate Vecs used when constructing AppVecs (which gave an additional ~5% speedup). Used the master branch of https://github.com/servo/rust-smallvec so we should wait with merging this until at least a new version on cargo is released. May also want to wait for rust 1.13 as this will likely be a regression in performance on stable otherwise.

SmallVec

test typecheck_prelude ... bench:  13,559,631 ns/iter (+/- 569,445)

Vec

test typecheck_prelude ... bench:  15,035,660 ns/iter (+/- 570,014)

brendanzab · 2016-10-05T00:52:36Z

check/src/unify_type.rs

-                        unifier.try_match(l, &Type::app(r.clone(), r_args[..offset].into()));
+
+                    let reduced_r = Type::app(r.clone(),
+                                              r_args[..offset].iter().cloned().collect());


Could you make this an intermediate? like:

let offset = r_args.len() - l_args.len(); let reduced_r_args = r_args[..offset].iter().cloned(); let reduced_r = Type::app(r.clone(), reduced_r_args.collect());

Just find the weird indentation kinda ugly.

I find it hard to read with to many superfluous intermediates as well (neither of the names reduced_r or reduced_r_args says anything). I will see what I can do, may be able to simply the code overall.

Agreed... no real 'best way' here. Not a big deal - I agree it might be a tad superfluous here.

shakes fist at Rust's ugly syntax and rightward drift

😆

brendanzab · 2016-10-05T00:53:26Z

check/tests/pass.rs

@@ -346,10 +348,10 @@ test
    let result = support::typecheck(text);
    assert!(result.is_ok(), "{}", result.unwrap_err());

-    let variants = Type::variants(vec![(support::intern_unscoped("T"),
+    let variants = Type::variants(collect![(support::intern_unscoped("T"),
                                        Type::function(vec![typ("a")],
                                                       support::typ_a("Test", vec![typ("a")])))]);


Indentation is now out of alignment.

I have got rustfmt set to format automatically but I think vec! is special cased, other macros do not get formatted...

Yep, that's right :(

brendanzab · 2016-10-05T00:54:02Z

base/src/types.rs

@@ -1212,7 +1225,7 @@ fn walk_move_types2<'a, I, F, T>(mut types: I, replaced: bool, output: &mut Vec<
            Some(typ) => {
                output.push(typ);
            }
-            None if replaced || !output.is_empty() => {
+            None if replaced || output.len() != 0 => {


VecLike from smallvec does not have an is_empty method but it appears this is only because the VecLike trait does not have the Deref bound specified correctly. Will submit a PR.

Could also just specify Deref<Target=[T]>myself which I am not sure why I did not do...

Dang - yeah, a PR would probably be useful there.

brendanzab · 2016-10-05T00:54:16Z

base/src/types.rs

    walk_move_types2(types.into_iter(), false, &mut out, &mut f);
-    if out.is_empty() {
+    if out.len() == 0 {


Since applications are usually done on just a few argument it is usually unnecessary to allocate a `Vec`. Using `SmallVec` this heap allocation can be avoided for applications with only a few arguments which should cover most of the cases.

Stable and beta cannot handle dev-dependencies in sub packages (cargo test -p gluon_parser)

Disabling since travis takes forever to start the osx builds

Marwes · 2016-11-10T22:13:49Z

Rebase. Since rust 1.13 is release there is no reason to delay merging this anymore I think. Disabled osx builds since they take forever to start.

brendanzab · 2016-11-10T22:59:09Z

base/src/types.rs

@@ -1077,12 +1085,14 @@ pub fn walk_move_type_opt<F: ?Sized, I, T>(typ: &Type<I, T>, f: &mut F) -> Optio
    }
 }

-pub fn walk_move_types<'a, I, F, T>(types: I, mut f: F) -> Option<Vec<T>>
+// FIXME Add R: Default and remove the `out` parameter
+pub fn walk_move_types<'a, I, F, T, R>(types: I, mut f: F) -> Option<R>


Could you elaborate on this comment?

Just forgot to remove that comment when removing the out parameter, fixed.

brendanzab · 2016-11-10T23:10:25Z

check/src/unify_type.rs

+            Ok(merge(r, new_type, r_args, new_args, Type::app))
+        }
+    }
+}


Love this - great idea breaking it out into another function ❤️

Marwes mentioned this pull request Aug 24, 2016

refactor(base): Remove the duplicate walk_move_type functions from in… #130

Merged

Marwes force-pushed the smallvec branch from de33d9a to 8c2cb1f Compare August 25, 2016 15:37

Marwes force-pushed the smallvec branch from b141701 to ab22a61 Compare October 4, 2016 16:39

Marwes force-pushed the smallvec branch from ab22a61 to a5b284d Compare October 4, 2016 17:05

brendanzab reviewed Oct 5, 2016

View reviewed changes

Marwes added 4 commits November 10, 2016 21:22

perf: Use SmallVec in the Type::App variant

a26c106

Since applications are usually done on just a few argument it is usually unnecessary to allocate a `Vec`. Using `SmallVec` this heap allocation can be avoided for applications with only a few arguments which should cover most of the cases.

perf: Allocate AppVec directly instead of using intermediate Vecs

b0bc41f

fix: Make collect-mac a required dependency

5e07700

Stable and beta cannot handle dev-dependencies in sub packages (cargo test -p gluon_parser)

refactor(check): Extract application unification to its own function

e5e8223

Marwes force-pushed the smallvec branch from 166566e to e5e8223 Compare November 10, 2016 21:00

chore: Disable osx builds

5f49e07

Disabling since travis takes forever to start the osx builds

brendanzab reviewed Nov 10, 2016

View reviewed changes

Marwes added 2 commits November 11, 2016 00:06

refactor: Remove old FIXME

8d8eca9

fix: Use smallvec from crates.io rather than github

0507200

brendanzab reviewed Nov 10, 2016

View reviewed changes

brendanzab merged commit 0b29da5 into master Nov 11, 2016

brendanzab deleted the smallvec branch November 11, 2016 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Use SmallVec in the Type::App variant #129

perf: Use SmallVec in the Type::App variant #129

Marwes commented Aug 24, 2016 •

edited

Loading

Marwes commented Aug 24, 2016

lambda-fairy commented Oct 3, 2016

Marwes commented Oct 3, 2016

Marwes commented Oct 4, 2016

brendanzab Oct 5, 2016

Marwes Oct 5, 2016

brendanzab Oct 5, 2016

brendanzab Oct 5, 2016

Marwes Oct 5, 2016 •

edited

Loading

brendanzab Oct 5, 2016

brendanzab Oct 5, 2016

Marwes Oct 5, 2016 •

edited

Loading

brendanzab Oct 5, 2016

brendanzab Oct 5, 2016

Marwes commented Nov 10, 2016

brendanzab Nov 10, 2016 •

edited

Loading

Marwes Nov 10, 2016

brendanzab Nov 11, 2016

brendanzab Nov 10, 2016

perf: Use SmallVec in the Type::App variant #129

perf: Use SmallVec in the Type::App variant #129

Conversation

Marwes commented Aug 24, 2016 • edited Loading

Marwes commented Aug 24, 2016

lambda-fairy commented Oct 3, 2016

Marwes commented Oct 3, 2016

Marwes commented Oct 4, 2016

SmallVec

Vec

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marwes Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marwes Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marwes commented Nov 10, 2016

brendanzab Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marwes commented Aug 24, 2016 •

edited

Loading

Marwes Oct 5, 2016 •

edited

Loading

Marwes Oct 5, 2016 •

edited

Loading

brendanzab Nov 10, 2016 •

edited

Loading