[libSyntax] Don't reference count RawSyntax #36165

ahoppen · 2021-02-25T19:19:15Z

Instead, only reference count the SyntaxArena that the RawSyntax nodes live in. The user of RawSyntax nodes must guarantee that the SyntaxArena stays alive as long as the RawSyntax nodes are being accessed.

During parse time, the SyntaxTreeCreator holds on to the SyntaxArena in which it creates RawSyntax nodes. When inspecting a syntax tree, the root SyntaxData node keeps the SyntaxArena alive. The change should be mostly invisible to the users of the public libSyntax API.

This change significantly decreases the overall reference-counting overhead. Since we were not able to free individual RawSyntax nodes anyway, performing the reference-counting on the level of the SyntaxArena feels natural.

70% of all changes are just adjustments in unittests/Syntax The actual logic change is fairly small (<400 LOC).

harlanhaskins · 2021-02-25T21:59:47Z

unittests/Syntax/StmtSyntaxTests.cpp

-  auto MinusOne = SyntaxFactory::makePrefixOperatorExpr(Minus,
-    SyntaxFactory::makeIntegerLiteralExpr(OneDigits));
+  RC<SyntaxArena> Arena = SyntaxArena::make();
+  auto ReturnKW = SyntaxFactory::makeReturnKeyword("", " ", Arena);


Not for this PR, but maybe it's time we make SyntaxFactory's methods instance methods and have it own the area in which new nodes are created...

Sounds like it might be a good idea. I’ll take a look at it later.

ahoppen · 2021-02-26T16:32:47Z

@swift-ci Please test

ahoppen · 2021-02-26T16:33:32Z

@swift-ci Please ASAN test

rintaro · 2021-02-26T19:38:28Z

lib/Syntax/SyntaxNodes.cpp.gyb

-    raw = ${make_missing_child(child)};
+    {
+      // make_missing_child access the 'Arena' variable. Create it.
+      const RC<SyntaxArena> &Arena = getRaw()->getArena();


Why this is reference &?

It used to be a reference to avoid ref-counting the arena. But of course it shouldn’t be since RawSyntax::getArena is no longer returning a reference. Good catch.

rintaro · 2021-02-26T19:44:28Z

include/swift/Syntax/RawSyntax.h

-            llvm::Optional<SyntaxNodeId> NodeId);
+  RawSyntax(SyntaxKind Kind, ArrayRef<const RawSyntax *> Layout,
+            size_t TextLength, SourcePresence Presence,
+            const RC<SyntaxArena> &Arena, llvm::Optional<SyntaxNodeId> NodeId);


Could you explain why Arena must be ref counted here`?

It doesn’t technically have to be. It’s more a design choice.

I always think of SyntaxArena as a ref-counted object and thus, we should write RC<SyntaxArena>. RawSyntax's use of SyntaxArena * is more like an unowned reference in Swift, which I consider an implementation detail of RawSyntax. As such, I decided that all methods on RawSyntax should operate on RC<SyntaxArena> instead of SyntaxArena *.

This also meant that the API is easier to use since we have isolated the conversion of RC<SyntaxArena> -> SyntaxArena * to two location instead of having to call RC<SyntaxArena>::get whenever invoking one of these functions.

Does this sound good to you or do you disagree?

I see. No objection. Thank you :)

rintaro · 2021-02-26T19:44:58Z

include/swift/Syntax/RawSyntax.h

-                    const RC<SyntaxArena> &Arena = SyntaxArena::make(),
+  static const RawSyntax *
+  make(SyntaxKind Kind, ArrayRef<const RawSyntax *> Layout, size_t TextLength,
+       SourcePresence Presence, const RC<SyntaxArena> &Arena,


rintaro · 2021-02-26T19:45:07Z

include/swift/Syntax/RawSyntax.h

+
+  static const RawSyntax *
+  makeAndCalcLength(SyntaxKind Kind, ArrayRef<const RawSyntax *> Layout,
+                    SourcePresence Presence, const RC<SyntaxArena> &Arena,


rintaro · 2021-02-26T22:03:31Z

include/swift/Syntax/SyntaxArena.h

+  /// The \c SyntaxArena's in this set are manually retained when added to this
+  /// list by \c addChildArena and manually released in the destructor of this
+  /// arena.
+  llvm::SmallPtrSet<SyntaxArena *, 4> ChildArenas;


Is there a mechanism to prevent circular references?

No, there’s not. But I can’t think of a real-world scenario where a circular reference might occur since newer arenas only ever reference older arenas (older = from a previous parsing run). Might be worth adding a check in assert builds though, just to be safe for the future.

rintaro · 2021-02-26T22:07:53Z

include/swift/Syntax/RawSyntax.h

    return make(TokKind, Text, /*TextLength=*/0, {}, {},
                SourcePresence::Missing, Arena);
  }

  /// @}

+  /// Return the arena in which this \c RawSyntax node has been allocated.
+  /// Keep in mind that the \c RawSyntax node *does not* retain the arena.
+  RC<SyntaxArena> getArena() const { return RC<SyntaxArena>(Arena); }


I feel like we should just return a pointer here.

rintaro · 2021-02-27T00:14:57Z

include/swift/Syntax/SyntaxData.h

+  /// Create a new root node
+  SyntaxData(AbsoluteRawSyntax AbsoluteRaw, const RC<SyntaxArena> &Arena)
+      : AbsoluteRaw(AbsoluteRaw), Parent(nullptr), Arena(Arena) {}


I think

SyntaxData(AbsoluteRawSyntax AbsoluteRaw) : AbsoluteRaw(AbsoluteRaw), Arena(AbsoluteRaw.getRaw()->getArena()) {}

is safer if there's no need to refer a different Arena.

You are absolutely right!

Instead, only reference count the SyntaxArena that the RawSyntax nodes live in. The user of RawSyntax nodes must guarantee that the SyntaxArena stays alive as long as the RawSyntax nodes are being accessed. During parse time, the SyntaxTreeCreator holds on to the SyntaxArena in which it creates RawSyntax nodes. When inspecting a syntax tree, the root SyntaxData node keeps the SyntaxArena alive. The change should be mostly invisible to the users of the public libSyntax API. This change significantly decreases the overall reference-counting overhead. Since we were not able to free individual RawSyntax nodes anyway, performing the reference-counting on the level of the SyntaxArena feels natural.

This fixes the SwiftSyntaxTests that were broken because the SyntaxFactory API now requires an Arena in which the nodes should be created.

ahoppen · 2021-03-01T08:44:31Z

@swift-ci Please test

swift-ci · 2021-03-01T11:31:36Z

Build failed
Swift Test OS X Platform
Git Sha - 9ff88b2

rintaro

This looks great!

rintaro · 2021-03-02T05:27:45Z

include/swift/Syntax/SyntaxData.h

  RC<RefCountedBox<SyntaxData>> Parent;

+  /// If this node is the root of a Syntax tree (i.e. \c Parent is \c nullptr ),
+  /// the arena in which this node's \c RawSyntax node has been allocated.
+  /// This keeps this \c RawSyntax nodes referenced by this tree alive.
+  RC<SyntaxArena> Arena;


Not in this PR: since Parent and Arena are mutually exclusive, it would be great if these can be a tagged union.

There’s a reason why it’s not a union. In the next PR, I’ll split SyntaxData into a SyntaxDataRef that doesn’t retain the arena (but has a parent) and a subclass SyntaxData which retains the arena. So I can’t easily reuse the storage.

ahoppen · 2021-03-02T12:24:36Z

@swift-ci Please test macOS

ahoppen · 2021-03-02T12:24:44Z

@swift-ci Please test Windows

harlanhaskins reviewed Feb 25, 2021

View reviewed changes

ahoppen force-pushed the pr/dont-ref-count-rawsyntax branch from 5ee74e1 to 1764ccf Compare February 26, 2021 16:27

ahoppen marked this pull request as ready for review February 26, 2021 16:32

ahoppen requested a review from rintaro February 26, 2021 16:32

rintaro reviewed Feb 27, 2021

View reviewed changes

ahoppen added 2 commits March 1, 2021 09:43

[libSyntax] Fix SwiftSyntaxTests broken due to change SyntaxFactory API

9ff88b2

This fixes the SwiftSyntaxTests that were broken because the SyntaxFactory API now requires an Arena in which the nodes should be created.

ahoppen force-pushed the pr/dont-ref-count-rawsyntax branch from 1764ccf to 9ff88b2 Compare March 1, 2021 08:44

rintaro approved these changes Mar 2, 2021

View reviewed changes

This was referenced Mar 2, 2021

[libSyntax] Miscellaneous minor improvements #36229

Merged

[libSyntax] Reference count SyntaxData #36230

Merged

This was referenced Mar 2, 2021

[libSyntax] Assert there are no reference-cycles in the SyntaxArenas #36232

Merged

[libSyntax] Make SyntaxFactory methods instance methods #36233

Merged

[libSyntax] Create deferred nodes in the ParsedRawSyntaxRecorder #36235

Merged

ahoppen merged commit 96cb756 into swiftlang:main Mar 2, 2021

ahoppen deleted the pr/dont-ref-count-rawsyntax branch March 2, 2021 16:40

[libSyntax] Don't reference count RawSyntax #36165

[libSyntax] Don't reference count RawSyntax #36165

Uh oh!

Conversation

ahoppen commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Feb 26, 2021

Uh oh!

ahoppen commented Feb 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Mar 1, 2021

Uh oh!

swift-ci commented Mar 1, 2021

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Mar 2, 2021

Uh oh!

ahoppen commented Mar 2, 2021

Uh oh!

Uh oh!

ahoppen commented Feb 25, 2021 •

edited

Loading