Prevent overflows by increasing ring buffer size #47
Conversation
Via trial and error, I could verify that all google-api-rs crates actually build if the ring-buffer is about 3 times larger than before. As the code is generated, some types are massive, and there is more complexity than in hand-written code. Increasing the buffer size should have no disadvantage, neither to performance, nor on memory requirements.
|
Does this belong in libsyntax itself? I'm not sure. |
|
It does, and I just pushed the respective commit to my fork. |
It seems, the most complex API, dfa-reporting, requires an even higher value. Reproduce this with ``` make dfareporting2d1-cli-cargo ARGS="build --no-default-features --features=with-syntex" ```
|
Looks good to me, I'll include it in my current release, since libsyntax has broken everything again. Do you know why this is now required though? Was it a change in google-apis, or in syntex? |
|
Unfortunately I am nearly totally clueless. The only thing I know is that google-apis-rs didn't change the way they generate code. It seems to have started though when upgrading from serde 0.6 to 0.7. Maybe now it generates different types that require much more space when pretty-printed ? Actually, the file generated by syntex has about 5000 empty lines right at the start, which might indicate something is quite wrong. The difference becomes striking when comparing the original linked above with the gist. It might be that there is some sort of bug in the pretty-printer causing this, and this fix just cures the symptoms. |
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.
|
I dug a little deeper, and here is what I found out. It might be interesting, if at some point we want to discover the true origin of the issue. 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 |
|
@erickt, @Byron I'm not sure about the root cause of the issue, but rusoto has gotten this issue as well (tracking at: rusoto/rusoto#248). In my testing, I re-ran a Travis CI build that built successfully earlier and it failed, so I can confirm that this issue is on syntex's/libsyntax's end. Also, the merged fix didn't solve the bug for us, but that might be something else going wrong on our end (e.g. the build not using the new syntex version) as I haven't really taken a look at it yet. |
|
@Byron actually could it not be fixed for rusoto because tripling the ring buffer isn't enough for rusoto? Cause our auto-generated code is several tens of thousands of lines IIRC. If so, seems like we should find the root cause of the issue and fix that. |
|
@indiv0 To me it seemed that factor depends on the size of the source code to be processed. The file that required this factor to be 55 was about 2.7MB in size. I was naively thinking that this was sufficient for the next decades to come, and do agree that it would be time to fix the issue properly. It should be possible to replace the static buffer with a dynamically allocated one, even without knowing all the details of the implementation. |
|
Would this be as straightforward as replacing the ring buffer with a |
Via trial and error, I could verify that all google-api-rs
crates actually build if the ring-buffer is about 3 times
larger than before.
As the code is generated, some types are massive, and there
is more complexity than in hand-written code.
Increasing the buffer size should have no disadvantage, neither
to performance, nor on memory requirements.
PS: If merged, it would be nice to get a point-release soon to allow google-apis-rs to build again on stable. Thank you.
Related to Byron/google-apis-rs#148,
Fixes #33
Fixes serde-rs/serde#229
Maybe related to dermesser/yup-oauth2#16