Skip to content

[Syntax] Don't pretty-print -emit-syntax JSON output #15380

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 1 commit into from
Mar 27, 2018
Merged

[Syntax] Don't pretty-print -emit-syntax JSON output #15380

merged 1 commit into from
Mar 27, 2018

Conversation

harlanhaskins
Copy link
Contributor

@harlanhaskins harlanhaskins commented Mar 20, 2018 •
edited
Loading

This patch turns off pretty-printing for swiftc -frontend -emit-syntax. Since these structures nest very deep, there's a LOT of wasted memory printing ' ' characters, and wasted parse time skipping them, which slows down deserialization in SwiftSyntax.

@harlanhaskins harlanhaskins requested a review from nkcsgexi March 20, 2018 20:48
@harlanhaskins harlanhaskins force-pushed the im-ugly-and-im-proud branch 2 times, most recently from 3553ff8 to eea8e29 Compare March 20, 2018 20:58
@nkcsgexi
Copy link
Contributor

Does it hurt to keep it pretty? The test is more readable that way.

@harlanhaskins
Copy link
Contributor Author

The test is definitely more readable, but it significantly increases the file size of the JSON, which makes a difference when deserializing.

Prettyprinted, serialize_multiple_decls.json is 5.6KB. Without prettyprinting, it's 1.9K.

@nkcsgexi
Copy link
Contributor

Yeah, i see your argument of keeping the serialization format slim, but can we make it optional? pretty-print can be opt-in for testing and debugging; and the default is the slim version for actual serialization.

@harlanhaskins
Copy link
Contributor Author

I could also just leave swift-syntax-test pretty.

@nkcsgexi
Copy link
Contributor

that's fair. Thanks!

@nkcsgexi
Copy link
Contributor

could you apply the same optimization for SourceKitd? it's https://github.com/apple/swift/blob/master/tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp#L1801

@harlanhaskins
Copy link
Contributor Author

@nkcsgexi Done 👍

@swift-ci please smoke test

@harlanhaskins
Copy link
Contributor Author

test/SourceKit/SyntaxTree/basic.swift compares the output of swift-syntax-test and SourceKit's JSON output. I'll add a -pretty-print-syntax option to swift-syntax-test to allow turning it off?

@harlanhaskins harlanhaskins force-pushed the im-ugly-and-im-proud branch 3 times, most recently from 8920cd7 to 737bd43 Compare March 22, 2018 16:03
@harlanhaskins
Copy link
Contributor Author

@swift-ci please smoke test

@nkcsgexi
Copy link
Contributor

Can we compare the output with Swiftc instead?

@harlanhaskins
Copy link
Contributor Author

@swift-ci please smoke test

@harlanhaskins
Copy link
Contributor Author

@swift-ci please smoke test

@harlanhaskins harlanhaskins merged commit 527cce4 into swiftlang:master Mar 27, 2018
@harlanhaskins harlanhaskins deleted the im-ugly-and-im-proud branch March 27, 2018 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkcsgexi nkcsgexi nkcsgexi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@harlanhaskins @nkcsgexi