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

Parser: Do not clone on peeking #78

Merged
merged 2 commits into from
Jul 11, 2017

Conversation

chris-m-h
Copy link
Contributor

This eliminates calls to clone() and to_owned() in the parser

  • Peeking now returns reference only
  • To obtain value, fetch_token needs to be called
  • The parser was adapted accordingly
  • Also: Pass anchor name by value to register_anchor

Christian Hofer added 2 commits June 21, 2017 10:20
This eliminates calls to clone() and to_owned() in the parser

- Peeking now returns reference only
- To obtain value, fetch_token needs to be called
- The parser was adapted accordingly
- Also: Pass anchor name by value to register_anchor
Also: Fix clippy errors
@hoodie
Copy link
Contributor

hoodie commented Jun 22, 2017

I'd really like to see how much this impacts performance

@chris-m-h
Copy link
Contributor Author

chris-m-h commented Jun 22, 2017

Using twitter.json from the JSON benchmark (http://json.rs/) I got the following bench results:

  • For a parser with an EventReceiver that simply ignores the events, I got a performance gain of 22%.
  • Including the creation of the YAML struct, I still got a performance gain of 15%.

I would regard that quite a difference, although I have to admit that it is less than I expected... I do not claim that the tests were performed under experimental conditions...


Let me add the data for parsing the yaml.org page itself - again just a very simple test...

  • Parser (with Scanner) 11% faster
  • Parser only (minus Scanner time): around 100% faster
  • Whole process including Yaml struct generation: 12 % faster

test parser::bench::bench_parser ... bench: 132,619 ns/iter (+/- 5,744)
test scanner::bench::bench_scanner ... bench: 117,354 ns/iter (+/- 19,291)
test yaml::test::bench_yaml ... bench: 149,222 ns/iter (+/- 17,655)

test parser::bench::bench_parser ... bench: 147,476 ns/iter (+/- 4,429)
test scanner::bench::bench_scanner ... bench: 116,219 ns/iter (+/- 3,479)
test yaml::test::bench_yaml ... bench: 167,065 ns/iter (+/- 11,638)

@chris-m-h
Copy link
Contributor Author

Any interest in this, @dtolnay or @chyh1990 ? Basically, just the internal structure of the parser functions had to be changed to stay inside the lifetime of the token references, in this way avoiding the need for cloning.

@chyh1990
Copy link
Owner

+1. Looks good to me. But it is a large patch, can we get another reviewer for it? @dtolnay

Copy link
Collaborator

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

LGTM as well.

@dtolnay dtolnay merged commit 81f5620 into chyh1990:master Jul 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants