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

separated_list doc and test update #1784

Merged

Conversation

SaltyKitkat
Copy link
Contributor

@SaltyKitkat SaltyKitkat requested a review from Geal as a code owner November 13, 2024 05:31
Copy link
Collaborator

@Geal Geal left a comment

Choose a reason for hiding this comment

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

this looks good, thanks

Copy link

codspeed-hq bot commented Dec 8, 2024

CodSpeed Performance Report

Merging #1784 will degrade performances by 11.95%

Comparing SaltyKitkat:separated_list_doc_and_test_update (587ba95) with main (5455747)

Summary

❌ 2 regressions
✅ 22 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main SaltyKitkat:separated_list_doc_and_test_update Change
float str 1.8 µs 2 µs -10.02%
number 215 ns 244.2 ns -11.95%

@Geal Geal merged commit fcc9f16 into rust-bakery:main Dec 8, 2024
16 of 17 checks passed
@gdennie
Copy link
Contributor

gdennie commented Dec 8, 2024

I haven't studied the code so perhaps it resolves other concerns or bugs but 11.95% seems like a big hit for functionality that seems to be available elsewise. Nonetheless, great. 😉

In a fantasy world we could have a marking trait, NonConsuming, that attaches to parsers derived from non-consuming subparsers that would be given the trait. That may mean an extended algebra of trait composition. Of course, for clarity we require restating the trait but it would be compiler checked. Then the trait would then make this constraint on the combinators compiler visible.

@Geal
Copy link
Collaborator

Geal commented Dec 8, 2024

This is a flaky benchmark so do not read too much into it

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.

3 participants