Avoid more allocations when compiling html5ever #37373

Merged
merged 3 commits into from Oct 29, 2016

Conversation

Projects
None yet
8 participants
@nnethercote
Contributor

nnethercote commented Oct 24, 2016

These three commits reduce the number of allocations performed when compiling html5ever from 13.2M to 10.8M, which speeds up compilation by about 2%.

r? @nrc

@@ -161,7 +162,7 @@ pub fn count_names(ms: &[TokenTree]) -> usize {
})
}
-pub fn initial_matcher_pos(ms: Rc<Vec<TokenTree>>, sep: Option<Token>, lo: BytePos)
+pub fn initial_matcher_pos(ms: Vec<TokenTree>, sep: Option<Token>, lo: BytePos)

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Oct 24, 2016

Contributor

I believe this means that we are now copying/moving the Vec; perhaps we only need &[TokenTree] here?

@Mark-Simulacrum

Mark-Simulacrum Oct 24, 2016

Contributor

I believe this means that we are now copying/moving the Vec; perhaps we only need &[TokenTree] here?

This comment has been minimized.

@nnethercote

nnethercote Oct 24, 2016

Contributor

The Vec gets put into the MatcherPos, so if it becomes a reference then MatcherPos will need a lifetime paramter and things get more complicated. Copying a Vec shouldn't be that expensive because only three words get copied, right? I did a Cachegrind run and this commit caused the number of instructions executed to drop.

@nnethercote

nnethercote Oct 24, 2016

Contributor

The Vec gets put into the MatcherPos, so if it becomes a reference then MatcherPos will need a lifetime paramter and things get more complicated. Copying a Vec shouldn't be that expensive because only three words get copied, right? I did a Cachegrind run and this commit caused the number of instructions executed to drop.

This comment has been minimized.

@oli-obk

oli-obk Oct 24, 2016

Contributor

Cloning a Vec copies all the allocated elements. But it can probably use memcpy, so that should be fast.

@oli-obk

oli-obk Oct 24, 2016

Contributor

Cloning a Vec copies all the allocated elements. But it can probably use memcpy, so that should be fast.

This comment has been minimized.

@nnethercote

nnethercote Oct 24, 2016

Contributor

I was under the impression that passing a Vec as an argument to a function doesn't clone it. Have I got that wrong?

@nnethercote

nnethercote Oct 24, 2016

Contributor

I was under the impression that passing a Vec as an argument to a function doesn't clone it. Have I got that wrong?

This comment has been minimized.

@oli-obk

oli-obk Oct 24, 2016

Contributor

oh right... forget I said anything, I thought the Rc was there to prevent the cloning.

@oli-obk

oli-obk Oct 24, 2016

Contributor

oh right... forget I said anything, I thought the Rc was there to prevent the cloning.

@leonardo-m

This comment has been minimized.

Show comment
Hide comment
@leonardo-m

leonardo-m Oct 24, 2016

Are those changes speeding up the compilation of other programs too? Optimizations should be added only after there's a more general proof of their quality.

Are those changes speeding up the compilation of other programs too? Optimizations should be added only after there's a more general proof of their quality.

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Oct 24, 2016

Contributor

It looks like SmallVec should impl Deref/DerefMut so that the .as_slice()/mut calls become redundant, and it's more of a drop in replacement for Vec.

Contributor

bluss commented Oct 24, 2016

It looks like SmallVec should impl Deref/DerefMut so that the .as_slice()/mut calls become redundant, and it's more of a drop in replacement for Vec.

src/libsyntax/ext/tt/macro_parser.rs
- .cloned()
- .collect()),
+ let mut cur_eis = SmallVector::zero();
+ cur_eis.push(initial_matcher_pos(ms.iter().cloned().collect(),

This comment has been minimized.

@oli-obk

oli-obk Oct 24, 2016

Contributor

this should be a simple ms.to_owned()

@oli-obk

oli-obk Oct 24, 2016

Contributor

this should be a simple ms.to_owned()

This comment has been minimized.

@nrc

nrc Oct 25, 2016

Member

And could this use SmallVector::one instead of zero and push?

@nrc

nrc Oct 25, 2016

Member

And could this use SmallVector::one instead of zero and push?

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Oct 25, 2016

Member

r=me with the nits in macro_parser.rs addressed

Member

nrc commented Oct 25, 2016

r=me with the nits in macro_parser.rs addressed

nnethercote added some commits Oct 21, 2016

Use `SmallVector` for the stack in `macro_parser::parse`.
This avoids 800,000 heap allocations when compiling html5ever.
Use `SmallVector` for `TtReader::stack`.
This avoids 800,000 heap allocations when compiling html5ever. It
requires tweaking `SmallVector` a little.
Don't use `Rc` in `TokenTreeOrTokenTreeVec`.
This avoids 800,000 allocations when compiling html5ever.
@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 25, 2016

Contributor

Updated to address comments, including adding Deref and DerefMut support to SmallVector, which made things nicer :)

Contributor

nnethercote commented Oct 25, 2016

Updated to address comments, including adding Deref and DerefMut support to SmallVector, which made things nicer :)

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Oct 25, 2016

Member

@bors: r+

Member

nrc commented Oct 25, 2016

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 25, 2016

Contributor

📌 Commit c440a7a has been approved by nrc

Contributor

bors commented Oct 25, 2016

📌 Commit c440a7a has been approved by nrc

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 26, 2016

Rollup merge of #37373 - nnethercote:html5ever-more-more, r=nrc
Avoid more allocations when compiling html5ever

These three commits reduce the number of allocations performed when compiling html5ever from 13.2M to 10.8M, which speeds up compilation by about 2%.

r? @nrc

bors added a commit that referenced this pull request Oct 26, 2016

Auto merge of #37416 - Manishearth:rollup, r=Manishearth
Rollup of 11 pull requests

- Successful merges: #37144, #37315, #37350, #37367, #37373, #37378, #37385, #37387, #37389, #37391, #37399
- Failed merges:
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 29, 2016

Contributor

⌛️ Testing commit c440a7a with merge e9b2fcb...

Contributor

bors commented Oct 29, 2016

⌛️ Testing commit c440a7a with merge e9b2fcb...

bors added a commit that referenced this pull request Oct 29, 2016

Auto merge of #37373 - nnethercote:html5ever-more-more, r=nrc
Avoid more allocations when compiling html5ever

These three commits reduce the number of allocations performed when compiling html5ever from 13.2M to 10.8M, which speeds up compilation by about 2%.

r? @nrc

@bors bors merged commit c440a7a into rust-lang:master Oct 29, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:html5ever-more-more branch Oct 30, 2016

@brson brson added the relnotes label Nov 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment