Skip to content

[libSyntax] Inline commonly called methods in RawSyntax and AbsoluteRawSyntax #36278

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 8, 2021
Merged

[libSyntax] Inline commonly called methods in RawSyntax and AbsoluteRawSyntax #36278

merged 2 commits into from
Mar 8, 2021

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Mar 4, 2021

These methods are small and setting up the stack frame etc. takes up the majority (or at least a significant amount) of their execution time. So let's give the compiler a chance to inline them.

@ahoppen
[libSyntax] Inline commonly called methods in RawSyntax and AbsoluteR…
95acf4d 
…awSyntax

These methods are super small and setting up the stack frame etc. takes
up the majority (or at least a significant amount) of their execution
time. So let's inline them.
@ahoppen ahoppen requested a review from rintaro March 4, 2021 09:50
@ahoppen
Copy link
Member Author

ahoppen commented Mar 4, 2021

@swift-ci Please smoke test

@ahoppen ahoppen mentioned this pull request Mar 4, 2021
Merged
rintaro
rintaro reviewed Mar 4, 2021
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about the code size. Especially, RawSyntax::make() (via makeAndCalcLength()) are called from many places including SyntaxFactory and SyntaxBuilder. Do you know the actual code side change?

include/swift/Syntax/RawSyntax.h Outdated
/// If the \p Str is not allocated in \p Arena, copy it to \p Arena and adjust
/// \p Str to point to the string's copy in \p Arena.
void copyToArenaIfNecessary(
StringRef &Str, const swift::RC<swift::syntax::SyntaxArena> &Arena) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a SyntaxArena method? I'm just uncomfortable with having an anonymous namespace in a header.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Then we could also use this to copy the entire source code over here: https://github.com/apple/swift/blob/main/lib/SyntaxParse/SyntaxTreeCreator.cpp#L42

@ahoppen
Copy link
Member Author

ahoppen commented Mar 5, 2021

I'm concerned about the code size

I just checked and defining the RawSyntax constructors and make function in the header does not seem to have a noticeable on file size (in release builds, didn’t try other variants)

Size of swift-frontend with constructors and make functions defined in 
RawSyntax.h:   94959944 bytes
RawSyntax.cpp: 94960688 bytes

AFAIU defining a function in a header (or alternatively making it inline) allows the compiler to inline the function, but it’s not required to do so if it doesn’t think it’s beneficial, which is presumably what happens in SyntaxFactory.

From https://en.cppreference.com/w/cpp/language/inline

Since this meaning of the keyword inline is non-binding, compilers are free to use inline substitution for any function that's not marked inline, and are free to generate function calls to any function marked inline. Those optimization choices do not change the rules regarding multiple definitions and shared statics listed above.

@ahoppen
Copy link
Member Author

ahoppen commented Mar 5, 2021

@swift-ci Please smoke test

@ahoppen
Copy link
Member Author

ahoppen commented Mar 5, 2021

@swift-ci Please smoke test macOS

rintaro
rintaro approved these changes Mar 5, 2021
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the binary size doesn't change much :)

@ahoppen ahoppen merged commit 107add7 into swiftlang:main Mar 8, 2021
@ahoppen ahoppen deleted the pr/inline-rawsyntax-methods branch March 8, 2021 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rintaro rintaro rintaro 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
@ahoppen @rintaro