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

Prevent overflows by increasing ring buffer size #33934

Merged
merged 1 commit into from
May 29, 2016
Merged

Prevent overflows by increasing ring buffer size #33934

merged 1 commit into from
May 29, 2016

Conversation

Byron
Copy link
Member

@Byron Byron commented May 28, 2016

Please note that this change is just done to prevent
issues as currently seen by syntex_syntax in future.
See serde-deprecated/syntex#47 for details.

As shown in serde-deprecated/syntex#33,
complex code can easily overflow the ring-buffer and
cause an assertion error.

Please note that this change is just done to prevent
issues as currently seen by syntex_syntax in future.
See serde-deprecated/syntex#47 for details.

As shown in serde-deprecated/syntex#33,
complex code can easily overflow the ring-buffer and
cause an assertion error.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@pnkfelix
Copy link
Member

i debated about asking for the comment to be updated with links to one or more of the issues discussing the choice of number here, ...

... but from reading those comment threads, it sounds like the number was discovered via blunt trial and error, and there isn't really much information on the threads about the basis for the number beyond the statement that trial and error was used to find it.

So I'll just r+ this and let the historians investigate the git blame output to try to find out where the number came from.

@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 29, 2016

📌 Commit 406378b has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented May 29, 2016

⌛ Testing commit 406378b with merge aee3073...

bors added a commit that referenced this pull request May 29, 2016
Prevent overflows by increasing ring buffer size

Please note that this change is just done to prevent
issues as currently seen by syntex_syntax in future.
See serde-deprecated/syntex#47 for details.

As shown in serde-deprecated/syntex#33,
complex code can easily overflow the ring-buffer and
cause an assertion error.
@Byron
Copy link
Member Author

Byron commented May 29, 2016

Unfortunately the new value 55 is just as magic as the value 3 it had before. For completeness, I will put some more information here, maybe someone will find it useful.

The library requiring such a high value is dfa-reporting (2.7MB), a generated file with plenty of highly-nested types. It seems like an assertion is triggered when the previously parsed library is pretty-printed. Please note that I didn't actually do it with the libsyntax implementation, but solely with syntex_syntax.
The latter will produce a file nearly 8MB in size, which has been enriched with implementations of various serde-traits.

As far as I know, the issues started showing up when upgrading from serde 0.6.13 to something newer. It clear that between these versions, syntex changed from v0.28.0 to v0.29.0. Here is what changed between these versions.

Chances are that this bug is already fixed in libsyntax, as syntex_syntax appears to be a little bit behind when it comes to the pretty-printing code.

@bors bors merged commit 406378b into rust-lang:master May 29, 2016
@indiv0
Copy link
Contributor

indiv0 commented Jun 17, 2016

@pnkfelix @Byron unfortunately the 55 factor isn't enough to solve this issue for rusoto, so I'm interested in finding a more long-term fix. I'm not exactly sure what the preferred method would be here. Finding the issue with pretty-print in the diff @Byron posted? Increasing the factor from 55? Changing to a dynamically allocated buffer?

I'm very interested in helping to solve this issue but I'm not sure what exactly I should do to tackle it.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2022
Abstract the pretty printer's ringbuffer to be infinitely sized

This PR backports dtolnay/prettyplease@8e5e83c from the `prettyplease` crate into `rustc_ast_pretty`.

Using a dedicated RingBuffer type with non-wrapping indices, instead of manually `%`-ing indices into a capped sized buffer, unlocks a number of simplifications to the pretty printing algorithm implementation in followup commits such as dtolnay/prettyplease@fcb5968 and dtolnay/prettyplease@4427ced.

This change also greatly reduces memory overhead of the pretty printer. The old implementation always grows its buffer to 205920 bytes even for files without deeply nested code, because it only wraps its indices when they hit the maximum tolerable size of the ring buffer (the size after which the pretty printer will crash if there are that many tokens buffered). In contrast, the new implementation uses memory proportional to the peak number of simultaneously buffered tokens only, not the number of tokens that have ever been in the buffer.

Speaking of crashing the pretty printer and "maximum tolerable size", the constant used for that in the old implementation is a lie:

https://github.com/rust-lang/rust/blob/de9b573eedaaa6d6e7c00c986cccbee802f9287b/compiler/rustc_ast_pretty/src/pp.rs#L227-L228

It was raised from 3 to 55 in rust-lang#33934 because that was empirically the size that avoided crashing on one particular test crate, but according to rust-lang#33934 (comment) other syntax trees still crash at that size. There is no reason to believe that any particular size is good enough for arbitrary code, and using a large number like 55 adds overhead to inputs that never need close to that much of a buffer. The new implementation eliminates this tradeoff.
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.

5 participants