Skip to content

Ignore proc-macro stdout to prevent IPC crash #8159

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 23, 2021
Merged

Ignore proc-macro stdout to prevent IPC crash #8159

merged 2 commits into from
Mar 23, 2021

Conversation

@edwin0cheng
Copy link
Contributor

@edwin0cheng edwin0cheng commented Mar 23, 2021

fixes #7954

r? @flodiebold

edwin0cheng
edwin0cheng commented Mar 23, 2021
View reviewed changes
@edwin0cheng edwin0cheng force-pushed the ignore-proc-macro-stdout branch from c176cea to e785aaf Compare March 23, 2021 10:51
lnicola
lnicola reviewed Mar 23, 2021
View reviewed changes
lnicola
lnicola reviewed Mar 23, 2021
View reviewed changes
@lnicola
Copy link
Member

lnicola commented Mar 23, 2021

Fun fact: we seem to do about 2080 proc macro expansions when analyzing RA itself, 166 of them with a response of more than 100 000 bytes, the largest being almost 600 KB.

@matklad
Copy link
Contributor

matklad commented Mar 23, 2021

the largest being almost 600 KB.

huh, who's that?

@lnicola
Copy link
Member

lnicola commented Mar 23, 2021

Um, no idea, something with __field22 and "documentSymbolProvider". Maybe some lsp-types thing?

image

lnicola
lnicola reviewed Mar 23, 2021
View reviewed changes
@matklad
Copy link
Contributor

matklad commented Mar 23, 2021

Yeah, looks like that's lsp_types::ServerCapabilites. Is this a single invocation, or all of the deserializes in the crate? If that's a single one, that feels excessive.

@lnicola
Copy link
Member

lnicola commented Mar 23, 2021

A single invocation, top ones are below. The total is 52 MB.

599139
534917
449627
366547
288330
274019
258890
256249
233323
218530
215860
199407
198340
197217
189853
187765
187585
181834
181535
181224
178820
178372
176892
165669
163620
163616
163493
163489
162706
161961
161521
161520
161435
161432
160896
160824
152180
150215
149944
149863

@bjorn3
Copy link
Member

bjorn3 commented Mar 23, 2021

Could we use something like bincode or rykv instead of json?

@lnicola
Copy link
Member

lnicola commented Mar 23, 2021

We could, but I'm not sure if it's slow enough to matter. We could also reuse the storage.

@matklad
Copy link
Contributor

matklad commented Mar 23, 2021

Yeah, cargo expand for lsp types produces 6.1mb out of 256kb, a factor of 25 blow-up. That probably isnt' completely unreasonable, but still...

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Mar 23, 2021 via email •
edited
Loading

@edwin0cheng edwin0cheng force-pushed the ignore-proc-macro-stdout branch from e785aaf to 9da069e Compare March 23, 2021 19:47
@edwin0cheng
Improve message usage in proc-macro
79f583e 
Reuse storage for the buffer send to child process of proc-macro.
@edwin0cheng edwin0cheng force-pushed the ignore-proc-macro-stdout branch from 9da069e to 79f583e Compare March 23, 2021 19:51
@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Mar 23, 2021 •
edited
Loading

I changed it to pass a buffer to reuse the storage. and let's handle the actual json size problem in another PR.

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Mar 23, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 23, 2021

Build succeeded:

@bors bors bot merged commit d702f10 into rust-lang:master Mar 23, 2021
@edwin0cheng edwin0cheng deleted the ignore-proc-macro-stdout branch March 23, 2021 20:22
@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Mar 24, 2021

Implemented #8182 for the method I mentioned above.

@lnicola
Copy link
Member

lnicola commented Mar 24, 2021

changelog fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lnicola lnicola lnicola left review comments

@flodiebold flodiebold flodiebold approved these changes

+1 more reviewer

@matklad matklad matklad left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RA crash when calling println! in proc-macro

5 participants

@edwin0cheng @lnicola @matklad @bjorn3 @flodiebold