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

Use SmallVector in CombineFields::instantiate. #37322

Merged
merged 1 commit into from
Oct 24, 2016

Conversation

nnethercote
Copy link
Contributor

This avoids 4% of malloc calls when compiling
rustc-benchmarks/issue-32278-big-array-of-strings, and 1--2% for other
benchmarks. A small win, but an easy one.

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@arielb1
Copy link
Contributor

arielb1 commented Oct 21, 2016

We are going to have a "better" SmallVec implementation soon-ish. Might be better to wait for it.

@nnethercote
Copy link
Contributor Author

How soon? Is there a PR? I have some other patches in the pipeline that convert Vec to SmallVector...

@arielb1
Copy link
Contributor

arielb1 commented Oct 21, 2016

There's #37270 and its SmallVec8.

@eddyb
Copy link
Member

eddyb commented Oct 21, 2016

@arielb1 It's not general purpose though. This one is also weird because the libsyntax thing is specifically geared towards [] | T | Vec<T>, i.e. what folding the AST needs (T -> T being the common case).

@nnethercote
Copy link
Contributor Author

I have filed #37371 for generalizing SmallVector. In the meantime, I think this PR should be evaluated on the assumption that SmallVector is a reasonable thing to use.

@@ -181,7 +182,7 @@ impl<'infcx, 'gcx, 'tcx> CombineFields<'infcx, 'gcx, 'tcx> {
a_is_expected: bool)
-> RelateResult<'tcx, ()>
{
let mut stack = Vec::new();
let mut stack = SmallVector::zero();
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment about the fact that the common case is one element?

This avoids 4% of malloc calls when compiling
rustc-benchmarks/issue-32278-big-array-of-strings, and 1--2% for other
benchmarks. A small win, but an easy one.
@eddyb
Copy link
Member

eddyb commented Oct 24, 2016

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Oct 24, 2016

📌 Commit 9270a92 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Oct 24, 2016

⌛ Testing commit 9270a92 with merge ac468b6...

bors added a commit that referenced this pull request Oct 24, 2016
Use `SmallVector` in `CombineFields::instantiate`.

This avoids 4% of malloc calls when compiling
rustc-benchmarks/issue-32278-big-array-of-strings, and 1--2% for other
benchmarks. A small win, but an easy one.
@bors bors merged commit 9270a92 into rust-lang:master Oct 24, 2016
@nnethercote nnethercote deleted the CombineFields-instantiate branch October 24, 2016 22:14
@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants