Avoid some allocations in the macro parser #37318

Merged
merged 3 commits into from Oct 22, 2016

Conversation

Projects
None yet
6 participants
@nnethercote
Contributor

nnethercote commented Oct 21, 2016

These three commits reduce the number of heap allocations done when compiling rustc-benchmarks/html5ever-2016-08-25 by 20%, from 16.5M to 13.3M. This speeds up (debug) compilation of it with a stage1 compiler by about 7%.

nnethercote added some commits Oct 20, 2016

Avoid an unnecessary clone in `macro_parser::parse`.
This avoids ~800,000 allocations in html5ever.
Avoid an unnecessary clone in `generic_extensions`.
This avoids ~800,000 allocations in html5ever.
@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 21, 2016

Contributor

r? @nrc

Contributor

nnethercote commented Oct 21, 2016

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned pnkfelix Oct 21, 2016

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Oct 21, 2016

Collaborator

r? @pnkfelix

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

Collaborator

rust-highfive commented Oct 21, 2016

r? @pnkfelix

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

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 21, 2016

Contributor

@eddyb tells me the 3rd commit is bogus, because those failure messages are not just used in panics.

Contributor

nnethercote commented Oct 21, 2016

@eddyb tells me the 3rd commit is bogus, because those failure messages are not just used in panics.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Oct 21, 2016

Member

r = me for the first two commits, but I agree with @eddyb that the third is a non-starter. Not sure how much he explained, but the parser used to use panic to give errors. That behaviour is mostly gone, but there are a few remnants left such as the macro parser, so this is an error message that is user facing whenever a macro doesn't apply.

Member

nrc commented Oct 21, 2016

r = me for the first two commits, but I agree with @eddyb that the third is a non-starter. Not sure how much he explained, but the parser used to use panic to give errors. That behaviour is mostly gone, but there are a few remnants left such as the macro parser, so this is an error message that is user facing whenever a macro doesn't apply.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 21, 2016

Contributor

@nrc: Thank you for the fast response. I've updated to fix the third commit. I had to use the turbofish in multiple calls like this ParseResult::<i32>::msg(tok) -- not sure if there's a better way to do that.

Contributor

nnethercote commented Oct 21, 2016

@nrc: Thank you for the fast response. I've updated to fix the third commit. I had to use the turbofish in multiple calls like this ParseResult::<i32>::msg(tok) -- not sure if there's a better way to do that.

src/libsyntax/ext/tt/macro_parser.rs
/// Fatal error (malformed macro?). Abort compilation.
Error(syntax_pos::Span, String)
}
+impl<T> ParseResult<T> {
+ pub fn msg(tok: Option<Token>) -> String {

This comment has been minimized.

@nrc

nrc Oct 21, 2016

Member

can you make this a method on &self rather than a static method? That should avoid the turbo fish.

@nrc

nrc Oct 21, 2016

Member

can you make this a method on &self rather than a static method? That should avoid the turbo fish.

This comment has been minimized.

@eddyb

eddyb Oct 21, 2016

Member

Can't because it needs to be used in several places, without a ParseResult around.

@eddyb

eddyb Oct 21, 2016

Member

Can't because it needs to be used in several places, without a ParseResult around.

This comment has been minimized.

@nrc

nrc Oct 21, 2016

Member

Then I would just make it a top-level function (with a more descriptive name), rather than a method

@nrc

nrc Oct 21, 2016

Member

Then I would just make it a top-level function (with a more descriptive name), rather than a method

This comment has been minimized.

@eddyb

eddyb Oct 21, 2016

Member

Indeed, I suggested failure_message on IRC (with that same descriptivity goal).

@eddyb

eddyb Oct 21, 2016

Member

Indeed, I suggested failure_message on IRC (with that same descriptivity goal).

This comment has been minimized.

@nnethercote

nnethercote Oct 21, 2016

Contributor

That's difficult, because the self is gone by the time the method is called in some cases. I'll make it a top-level fn instead.

@nnethercote

nnethercote Oct 21, 2016

Contributor

That's difficult, because the self is gone by the time the method is called in some cases. I'll make it a top-level fn instead.

src/libsyntax/ext/tt/macro_rules.rs
@@ -227,7 +231,11 @@ pub fn compile(sess: &ParseSess, def: &ast::MacroDef) -> SyntaxExtension {
let argument_map = match parse(sess, &Vec::new(), arg_reader, &argument_gram) {
Success(m) => m,
- Failure(sp, str) | Error(sp, str) => {
+ Failure(sp, tok) => {
+ let str = ParseResult::<i32>::msg(tok);

This comment has been minimized.

@nrc

nrc Oct 21, 2016

Member

nit: str is a bad name because it is a very common type, so the code is a little jarring.

@nrc

nrc Oct 21, 2016

Member

nit: str is a bad name because it is a very common type, so the code is a little jarring.

This comment has been minimized.

@nnethercote

nnethercote Oct 21, 2016

Contributor

I'll use s.

@nnethercote

nnethercote Oct 21, 2016

Contributor

I'll use s.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 21, 2016

Contributor

All comments addressed!

Contributor

nnethercote commented Oct 21, 2016

All comments addressed!

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 21, 2016

Member

@nnethercote There's still the matter of using Token::Eof instead of None. Right now Some(Token::Eof) is an impossible case so the Option is wasteful.

Member

eddyb commented Oct 21, 2016

@nnethercote There's still the matter of using Token::Eof instead of None. Right now Some(Token::Eof) is an impossible case so the Option is wasteful.

Replace the `String` in `ParseResult::Failure` with `Token`.
This lets us delay creation of failure messages until they are needed,
which avoids ~1.6M allocations in html5ever.
@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 21, 2016

Contributor

There's still the matter of using Token::Eof instead of None

Oh, that was a comment on IRC and I missed it. Thanks for the suggestion, I've updated again.

Contributor

nnethercote commented Oct 21, 2016

There's still the matter of using Token::Eof instead of None

Oh, that was a comment on IRC and I missed it. Thanks for the suggestion, I've updated again.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 21, 2016

Member

@bors r=nrc,eddyb

Member

eddyb commented Oct 21, 2016

@bors r=nrc,eddyb

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 21, 2016

Contributor

📌 Commit b817cf8 has been approved by nrc,eddyb

Contributor

bors commented Oct 21, 2016

📌 Commit b817cf8 has been approved by nrc,eddyb

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

Rollup merge of #37318 - nnethercote:html5ever-more, r=nrc,eddyb
Avoid some allocations in the macro parser

These three commits reduce the number of heap allocations done when compiling rustc-benchmarks/html5ever-2016-08-25 by 20%, from 16.5M to 13.3M. This speeds up (debug) compilation of it with a stage1 compiler by about 7%.

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

Rollup merge of #37318 - nnethercote:html5ever-more, r=nrc,eddyb
Avoid some allocations in the macro parser

These three commits reduce the number of heap allocations done when compiling rustc-benchmarks/html5ever-2016-08-25 by 20%, from 16.5M to 13.3M. This speeds up (debug) compilation of it with a stage1 compiler by about 7%.

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

Auto merge of #37341 - Manishearth:rollup, r=Manishearth
Rollup of 8 pull requests

- Successful merges: #37294, #37298, #37301, #37310, #37318, #37321, #37326, #37327
- Failed merges:
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 22, 2016

Contributor

⌛️ Testing commit b817cf8 with merge a117bba...

Contributor

bors commented Oct 22, 2016

⌛️ Testing commit b817cf8 with merge a117bba...

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

Auto merge of #37318 - nnethercote:html5ever-more, r=nrc,eddyb
Avoid some allocations in the macro parser

These three commits reduce the number of heap allocations done when compiling rustc-benchmarks/html5ever-2016-08-25 by 20%, from 16.5M to 13.3M. This speeds up (debug) compilation of it with a stage1 compiler by about 7%.

@bors bors merged commit b817cf8 into rust-lang:master Oct 22, 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 branch Oct 23, 2016

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