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 benchmarking #49

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

InnocentZero
Copy link
Contributor

No description provided.

Signed-off-by: innocentzero <[email protected]>
Signed-off-by: innocentzero <[email protected]>
Signed-off-by: innocentzero <[email protected]>
Signed-off-by: innocentzero <[email protected]>
@InnocentZero
Copy link
Contributor Author

I'll be adding more, but I wanted initial feedback if any.

@kubouch
Copy link
Contributor

kubouch commented Jan 23, 2025

Nice, new benchmarks are good. It would be good to comment on what each benchmark is supposed to measure. For example, the combined benchmark is supposed to give a general run-down of the parser's features.

Another thing I've wondered is that instead of having a 10x/100x/1000x copy of each benchmark, there could be some generator code that that takes the 1x version and multiplies it Nx, incrementing some number with search and replace, for example.

@InnocentZero
Copy link
Contributor Author

I'll add the comments, sure. As for the 100x/1000x variants, I was checking out nushell's main repo and it seems that they are benchmarking it differently. Maybe we can just copy 1x file contents to the buffer and repeat it 10x/100x times to generate the testcases internally?

@kubouch
Copy link
Contributor

kubouch commented Jan 26, 2025

Maybe we can just copy 1x file contents to the buffer and repeat it 10x/100x times to generate the testcases internally?

Yeah, that's what I meant. Copy the 1x version into a buffer in memory and parse that. But there needs to be some search and replace because sometimes duplicated content is not allowed (e.g., the same command defined twice in the same file, there needs to be foo1, foo2, ..., foo100 -- this is not yet implemented in this parser, but it's that way in Nushell).

@kubouch
Copy link
Contributor

kubouch commented Mar 5, 2025

@InnocentZero Do you plan continuing this?

@InnocentZero
Copy link
Contributor Author

InnocentZero commented Mar 6, 2025

Hi, really sorry for this getting stagnated. Personally I'd like to continue but RL is getting a bit hectic rn so I can't commit to it properly. I did try looking around for solutions, but wasn't able to write any significant code. If you want to, feel free to close the issue, and I'll open a new one when I get back to this.

@kubouch
Copy link
Contributor

kubouch commented Mar 6, 2025

No worries, you can continue whenever you have time.

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.

2 participants