Skip to content

[libSyntax] Don't create dedicated deferred nodes in SyntaxTreeCreator #36364

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.

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

Merged
merged 1 commit into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions include/swift/Parse/SyntaxParseActions.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ struct DeferredNodeInfo {
// MARK: - SyntaxParseActions

class SyntaxParseActions {
llvm::BumpPtrAllocator DeferredNodeAllocator;

virtual void _anchor();

public:
Expand Down Expand Up @@ -111,24 +109,24 @@ class SyntaxParseActions {
StringRef leadingTrivia,
StringRef trailingTrivia,
CharSourceRange range,
bool isMissing);
bool isMissing) = 0;

/// Create a deferred layout node that may or may not be recorded later using
/// \c recordDeferredLayout. The \c SyntaxParseAction is responsible for
/// keeping the deferred token alive until it is destructed.
virtual OpaqueSyntaxNode
makeDeferredLayout(syntax::SyntaxKind k, bool isMissing,
const ArrayRef<RecordedOrDeferredNode> &children);
const ArrayRef<RecordedOrDeferredNode> &children) = 0;

/// Record a deferred token node that was previously created using \c
/// makeDeferredToken. The deferred data will never be used again, so it can
/// be destroyed by this method. Note that not all deferred nodes will be
/// recorded and that pending deferred nodes need to be freed when the \c
/// SyntaxParseActions is destructed.
virtual OpaqueSyntaxNode recordDeferredToken(OpaqueSyntaxNode deferred);
virtual OpaqueSyntaxNode recordDeferredToken(OpaqueSyntaxNode deferred) = 0;

/// Record a deferred layout node. See recordDeferredToken.
virtual OpaqueSyntaxNode recordDeferredLayout(OpaqueSyntaxNode deferred);
virtual OpaqueSyntaxNode recordDeferredLayout(OpaqueSyntaxNode deferred) = 0;

/// Since most data of \c ParsedRawSyntax is described as opaque data, the
/// \c ParsedRawSyntax node needs to reach out to the \c SyntaxParseAction,
Expand All @@ -140,11 +138,11 @@ class SyntaxParseActions {
/// node starts at \p StartLoc.
virtual DeferredNodeInfo getDeferredChild(OpaqueSyntaxNode node,
size_t childIndex,
SourceLoc startLoc);
SourceLoc startLoc) = 0;

/// Return the number of children, \p node has. These can be retrieved using
/// \c getDeferredChild.
virtual size_t getDeferredNumChildren(OpaqueSyntaxNode node);
virtual size_t getDeferredNumChildren(OpaqueSyntaxNode node) = 0;

/// Attempt to realize an opaque raw syntax node for a source file into a
/// SourceFileSyntax node. This will return \c None if the parsing action
Expand Down
21 changes: 20 additions & 1 deletion include/swift/SyntaxParse/SyntaxTreeCreator.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class SourceFileSyntax;
///
/// It also handles caching re-usable RawSyntax objects and skipping parsed
/// nodes via consulting a \c SyntaxParsingCache.
class SyntaxTreeCreator: public SyntaxParseActions {
class SyntaxTreeCreator final : public SyntaxParseActions {
SourceManager &SM;
unsigned BufferID;
RC<syntax::SyntaxArena> Arena;
Expand All @@ -48,6 +48,8 @@ class SyntaxTreeCreator: public SyntaxParseActions {
/// tree.
SyntaxParsingCache *SyntaxCache;

llvm::BumpPtrAllocator ScratchAlloc;
Copy link
Member

Choose a reason for hiding this comment

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

This is unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. I’ll delete it in the next PR. Thanks!


public:
SyntaxTreeCreator(SourceManager &SM, unsigned bufferID,
SyntaxParsingCache *syntaxCache,
Expand All @@ -70,6 +72,23 @@ class SyntaxTreeCreator: public SyntaxParseActions {

std::pair<size_t, OpaqueSyntaxNode>
lookupNode(size_t lexerOffset, syntax::SyntaxKind kind) override;

OpaqueSyntaxNode makeDeferredToken(tok tokenKind, StringRef leadingTrivia,
StringRef trailingTrivia,
CharSourceRange range,
bool isMissing) override;

OpaqueSyntaxNode
makeDeferredLayout(syntax::SyntaxKind k, bool IsMissing,
const ArrayRef<RecordedOrDeferredNode> &children) override;

OpaqueSyntaxNode recordDeferredToken(OpaqueSyntaxNode deferred) override;
OpaqueSyntaxNode recordDeferredLayout(OpaqueSyntaxNode deferred) override;

DeferredNodeInfo getDeferredChild(OpaqueSyntaxNode node, size_t ChildIndex,
SourceLoc StartLoc) override;

size_t getDeferredNumChildren(OpaqueSyntaxNode node) override;
};

} // end namespace swift
Expand Down
1 change: 0 additions & 1 deletion lib/Parse/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ add_swift_host_library(swiftParse STATIC
ParseStmt.cpp
ParseType.cpp
PersistentParserState.cpp
SyntaxParseActions.cpp
SyntaxParsingCache.cpp
SyntaxParsingContext.cpp)
_swift_gyb_target_sources(swiftParse PRIVATE
Expand Down
179 changes: 0 additions & 179 deletions lib/Parse/SyntaxParseActions.cpp

This file was deleted.

89 changes: 89 additions & 0 deletions lib/SyntaxParse/SyntaxTreeCreator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,92 @@ SyntaxTreeCreator::lookupNode(size_t lexerOffset, syntax::SyntaxKind kind) {
size_t length = raw->getTextLength();
return {length, static_cast<OpaqueSyntaxNode>(raw)};
}

OpaqueSyntaxNode SyntaxTreeCreator::makeDeferredToken(tok tokenKind,
StringRef leadingTrivia,
StringRef trailingTrivia,
CharSourceRange range,
bool isMissing) {
// Instead of creating dedicated deferred nodes that will be recorded only if
// needed, the SyntaxTreeCreator always records all nodes and forms RawSyntax
// nodes for them. This eliminates a bunch of copies that would otherwise
// be required to record the deferred nodes.
// Should a deferred node not be recorded, its data stays alive in the
// SyntaxArena. This causes a small memory leak but since most nodes are
// being recorded, it is acceptable.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a TODO comments about creating deferred nodes in a different arena

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided against creating deferred nodes in a different arena, because I assume that creating a new arena is rather expensive (heap allocation of the arena itself plus another heap allocation for its bump allocator) and because I think the leakage is pretty small (see my description of the PR). But disabling deferred node creation in the backtracking scopes that are never cancelled sounds like a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

After offline discussion, we decided to merge this as it and I’ll set up a follow-up PR that disables the SyntaxParsingContext when it’s not needed.

if (isMissing) {
auto Node = recordMissingToken(tokenKind, range.getStart());
return Node;
} else {
auto Node = recordToken(tokenKind, leadingTrivia, trailingTrivia, range);
return Node;
}
}

OpaqueSyntaxNode SyntaxTreeCreator::makeDeferredLayout(
syntax::SyntaxKind k, bool IsMissing,
const ArrayRef<RecordedOrDeferredNode> &children) {
SmallVector<OpaqueSyntaxNode, 16> opaqueChildren;
opaqueChildren.reserve(children.size());

for (size_t i = 0; i < children.size(); ++i) {
opaqueChildren.push_back(children[i].getOpaque());
}

// Also see comment in makeDeferredToken
return recordRawSyntax(k, opaqueChildren);
}

OpaqueSyntaxNode
SyntaxTreeCreator::recordDeferredToken(OpaqueSyntaxNode deferred) {
// We don't diffirentiate between deferred and recorded nodes. See comment in
// makeDeferredToken.
return deferred;
}

OpaqueSyntaxNode
SyntaxTreeCreator::recordDeferredLayout(OpaqueSyntaxNode deferred) {
// We don't diffirentiate between deferred and recorded nodes. See comment in
// makeDeferredToken.
return deferred;
}

DeferredNodeInfo SyntaxTreeCreator::getDeferredChild(OpaqueSyntaxNode node,
size_t ChildIndex,
SourceLoc StartLoc) {
const RawSyntax *raw = static_cast<const RawSyntax *>(node);

// Compute the start offset of the child node by advancing StartLoc by the
// length of all previous child nodes.
for (unsigned i = 0; i < ChildIndex; ++i) {
const RawSyntax *child = raw->getChild(i);
if (child) {
StartLoc = StartLoc.getAdvancedLoc(child->getTextLength());
}
}

const RawSyntax *Child = raw->getChild(ChildIndex);
if (Child == nullptr) {
return DeferredNodeInfo(
RecordedOrDeferredNode(nullptr, RecordedOrDeferredNode::Kind::Null),
syntax::SyntaxKind::Unknown, tok::NUM_TOKENS, /*IsMissing=*/false,
CharSourceRange(StartLoc, /*Length=*/0));
} else if (Child->isToken()) {
return DeferredNodeInfo(
RecordedOrDeferredNode(Child,
RecordedOrDeferredNode::Kind::DeferredToken),
syntax::SyntaxKind::Token, Child->getTokenKind(), Child->isMissing(),
CharSourceRange(StartLoc, Child->getTextLength()));
} else {
return DeferredNodeInfo(
RecordedOrDeferredNode(Child,
RecordedOrDeferredNode::Kind::DeferredLayout),
Child->getKind(), tok::NUM_TOKENS,
/*IsMissing=*/false, CharSourceRange(StartLoc, Child->getTextLength()));
}
}

size_t SyntaxTreeCreator::getDeferredNumChildren(OpaqueSyntaxNode node) {
const syntax::RawSyntax *raw = static_cast<const syntax::RawSyntax *>(node);
return raw->getNumChildren();
}
Loading