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

Add combine based benchmark to catch exponential trait selection #614

Closed
wants to merge 1 commit into from

Conversation

Marwes
Copy link

@Marwes Marwes commented Feb 4, 2020

Similar to #588 but shows a newer regression and without pulling in combine itself.

Reduced to run in ~1.5s locally, could be lowered or increased by adding more parsers to the chain.

@Marwes
Copy link
Author

Marwes commented Feb 4, 2020

cc rust-lang/rust#68666

@Marwes Marwes changed the title Add combine based benchmark to catch exponential regression Add combine based benchmark to catch exponential trait selection Feb 4, 2020
@nnethercote
Copy link
Contributor

You said "newer regression" -- is there another regression I'm not aware of?

@Marwes
Copy link
Author

Marwes commented Feb 5, 2020

rust-lang/rust#67454 and rust-lang/rust#68666 is the ones I refer to, but I probably should not have used regression here. The issues contain practically the same code, only the Parser trait has been altered slightly as one is based on combine-3 and the newer on combine-4.

So it has regressed between combine versions, but it hasn't regressed between rust versions. Sorry for the confusion!

@Mark-Simulacrum
Copy link
Member

I'll leave this up to @nnethercote to decide on merging or not, I'm personally not sure but have no specific concerns.

@Marwes
Copy link
Author

Marwes commented Feb 5, 2020

It might be better to merge this after rust-lang/rust#68666 has been fixed, in which case the number of "parser"s could be increased without penalty to the benchmarks.

@nnethercote
Copy link
Contributor

It might be better to merge this after rust-lang/rust#68666 has been fixed

My understanding is that #68666 (and #67454) were both fixed by #67471, which reverted the changes from #66405 that caused the regression. Furthermore, the regression fix was backported to beta. (#68666 was not marked as fixed, though; I did that just now.)

So I think you could increase the number of parsers safely now. But 1.5s sounds fine, and as long as it demonstrates the original regression (i.e. the old code is drastically slower) I think the current number of parsers should be fine.

@Marwes
Copy link
Author

Marwes commented Feb 5, 2020

So I think you could increase the number of parsers safely now. But 1.5s sounds fine, and as long as it demonstrates the original regression (i.e. the old code is drastically slower) I think the current number of parsers should be fine.

I can't, since this is a separate performance issue.

@rylev
Copy link
Member

rylev commented Jul 9, 2021

Going to close since this is a bit stale. Feel free to reopen if you'd like to continue working on this (or if you think this should be rebased and re-reviewed).

@rylev rylev closed this Jul 9, 2021
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