Skip to content

Remove unused nodes from SyntaxTreeDeserializer.nodeLookupTable#3

Closed
ahoppen wants to merge 2 commits intoswiftlang:masterfrom
ahoppen:clean-nodelookuptable
Closed

Remove unused nodes from SyntaxTreeDeserializer.nodeLookupTable#3
ahoppen wants to merge 2 commits intoswiftlang:masterfrom
ahoppen:clean-nodelookuptable

Conversation

@ahoppen
Copy link
Copy Markdown
Member

@ahoppen ahoppen commented Aug 30, 2018

At the moment we keep a reference to all nodes in the nodeLookupTable of SyntaxTreeDeserializer. The memory usage of SyntaxTreeDeserializer always grows over time. After this PR we will remove nodes from nodeLookupTable that will never be used for incremental deserialisation.

Implements rdar://43516167

@rintaro
Copy link
Copy Markdown
Member

rintaro commented Sep 11, 2018

@rintaro
Copy link
Copy Markdown
Member

rintaro commented Sep 11, 2018

@ahoppen Just to clarify: Even with this changes, nodeLookupTable itself is still indefinitely growing, right? This change only frees the "value" of the entries.

@ahoppen
Copy link
Copy Markdown
Member Author

ahoppen commented Sep 12, 2018

Yes, that's true.

It only now occurred to me that maybe every RawSyntax node should have a weak reference to its SyntaxTreeDeserializer. That way it can remove itself from the nodeLookupTable once it gets destroyed. What do you think?

@rintaro
Copy link
Copy Markdown
Member

rintaro commented Sep 12, 2018

I don't think that is a good idea. RawSyntax should not know how itself is managed.

A simple question is: why not just recreating the table?

   private func deserializeByteTree(_ data: Data) throws -> RawSyntax {
     var userInfo: [ByteTreeUserInfoKey: Any] = [:]
     var newLookupTable: [SyntaxNodeId: RawSyntax] = [:]
     userInfo[.rawSyntaxDecodedCallback] = { (node: RawSyntax) -> Void in
       newLookupTable[node.id] = node
     }

     /* ... actual deserialization work .. */

     self.lookupTable = newLookupTable
   }

@ahoppen
Copy link
Copy Markdown
Member Author

ahoppen commented Sep 12, 2018

Because if a node gets reused, we won't visit its children that way. And we definitely don't want to do that because it would get us back to O(n) incremental parsing.

…kupTable

This way we don't continue to retain RawSyntax nodes that are no longer
needed for incremental transfer.
@ahoppen ahoppen force-pushed the clean-nodelookuptable branch from b40e835 to 0b303c0 Compare September 12, 2018 07:38
@rintaro
Copy link
Copy Markdown
Member

rintaro commented Sep 12, 2018

Ah, I understand, thanks. Let me think for a while...

Copy link
Copy Markdown
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.

OK, let's merge this AS IS for now. This is definitely an improvement.
We can do further improvement later.
@nkcsgexi Could you review this?

@nkcsgexi
Copy link
Copy Markdown
Contributor

I'm still reviewing this. Please hold on merging.

@nkcsgexi
Copy link
Copy Markdown
Contributor

I was also worrying about the ever increasing size of nodeLookupTable and couldn't figure out an efficient way to clear it up. I think these dangling weak pointers should not be a problem in practice, but could you add some comments there explaining the effect?

@rintaro
Copy link
Copy Markdown
Member

rintaro commented Sep 22, 2018

Included in #11. Closing

@rintaro rintaro closed this Sep 22, 2018
@ahoppen ahoppen deleted the clean-nodelookuptable branch October 3, 2019 14:55
CippoX added a commit to CippoX/swift-syntax that referenced this pull request Mar 21, 2023
# This is the 1st commit message:

fixed testAvailabilityQuery34 and testAvailabilityQueryUnavailability28

# This is the commit message swiftlang#2:

Update Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift

Co-authored-by: Kim de Vos <kimdevos12@hotmail.com>
# This is the commit message swiftlang#3:

added fixedSource in test case

# This is the commit message swiftlang#4:

minor changes

# This is the commit message swiftlang#5:

implemented recovery inside the parser

# This is the commit message swiftlang#6:

runned format.py

# This is the commit message swiftlang#7:

minor changes

# This is the commit message swiftlang#8:

minor changes
adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024
Make members of a private extension fileprivate.
adevress pushed a commit to adevress/swift-syntax that referenced this pull request Jan 14, 2024
Adding line breaks inside of #if conditionals is dangerous because line breaks are only valid if the entire condition is wrapped in parens. It's difficult to conditionally add parens only if a break is necessary because it implies backtracking into the printer's output buffer to insert the opening paren.

Our options for fixing this problem were:
1. Implement paren wrapping the #if condition if a break fires (very complex).
2. Always paren wrap the #if condition (looks bad in the most common case where there's no breaking).
3. Disable breaking in the pretty print but allow users to explicitly break in the condition using discretionary newlines.

I selected option swiftlang#3 and implemented by adding new kind of token known as "printer control" to disable/enable suppressing of break tokens. These printer control tokens can be used in other scenarios, such as suppressing breaks in string interpolation expressions.

Fixes SR-11815
kateinoigakukun pushed a commit to kateinoigakukun/swift-syntax that referenced this pull request Feb 27, 2024
ci: Change runner from macOS to Linux
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.

3 participants