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

Large tuples are a performance footgun #237

Open
2 tasks done
epage opened this issue Apr 27, 2023 · 3 comments
Open
2 tasks done

Large tuples are a performance footgun #237

epage opened this issue Apr 27, 2023 · 3 comments
Labels
A-combinator Area: combinators C-bug Category: Things not working as expected M-breaking-change Meta: Implementing or merging this will introduce a breaking change.

Comments

@epage
Copy link
Collaborator

epage commented Apr 27, 2023

Please complete the following tasks

rust version

1.68

winnow version

0.4.0

Minimal reproducible code

(a, b).map(...).parse_next(input)

Steps to reproduce the bug with the above code

TBD

Actual Behaviour

Slow

Expected Behaviour

Fast

Additional Context

See #230

Options

@epage epage added A-combinator Area: combinators C-bug Category: Things not working as expected labels Apr 27, 2023
@epage epage modified the milestones: 0.4.x, 0.5.x Apr 27, 2023
@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Apr 27, 2023
@epage
Copy link
Collaborator Author

epage commented Jun 16, 2023

One thing that further decreases performance are large output types. Surprisingly, I've seen quite big performance improvements by Box-ing large types that are passed though multiple layers of parser code. Of course, boxing by default is a performance footgun and usually makes the parser substantially slower.

In addition to that, tuple parsers should be used carefully as they can run into the same performance issues when multiple large output types are involved or if they simply have too many items in them.

E.g. i replaced something like

(a, b).map(...).parse_next(input)

with

let (input, a) = a.parse_next(input)?;
...
let (input, b) = b.parse_next(input)?;
...
Ok((input, ...))

in quite a few places to resolve performance issues.

I wonder if #251 improved the situation as the compiler is now more likely to optimize the tuple version into the imperative version.

@epage
Copy link
Collaborator Author

epage commented Jun 16, 2023

@martinohmann when you have more time, could you create a branch where hcl-edit is using tuples so I can do some more analysis of this? I'd like to see how #251 or tuple alternatives may be able to help improve things. Because of the chance of this being fixed in #251, I'm deprioritizing this for now, so no rush.

@martinohmann
Copy link
Contributor

Good idea! I'll try find some tuple cases. The parser is built in a way now that makes bringing these back a bit more involved. But I think I see 1-3 cases that are "easy" to negatively impact performance. Not sure if I can get to it this month or next month. Will ping you once I have a branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-combinator Area: combinators C-bug Category: Things not working as expected M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

2 participants