Bump allocated RawSyntax #544

rintaro · 2022-08-02T23:03:52Z

Introduce SyntaxArena that manages the memory of RawSyntax
RawSyntaxData is the "data" of raw syntax managed by SyntaxArena, and it has
- Unowned pointer to the SyntaxArena
- Tagged union of "token" or "layout" data where all data are managed by the SyntaxArena
RawSyntax is now just a pointer wrapper of RawSyntaxData and APIs.
Root SyntaxData now owns the SyntaxArena of the root RawSyntax.

For intermediate solution SyntaxArena has static var default which is used by Factory methods. This cannot be a permanent solution because it's never deallocate the memory. In future all factory methods will require explicit arena to use.

CodaFi · 2022-08-05T20:24:41Z

Sources/SwiftSyntax/gyb_generated/Trivia.swift

    switch kind {
    case 0:
-      return false
+      return .spaces(text.count / 1)


Can we not emit a division when the base is 1 like this?

CodaFi · 2022-08-05T20:26:00Z

Sources/SwiftSyntax/RawSyntax+CSwiftSyntax.swift

@@ -0,0 +1,113 @@
+//===------------------ RawSyntax.swift - Raw Syntax nodes ----------------===//


Nit: Header should reflect the file name.

* Introduce 'SyntaxArena' that manages the memory of RawSyntax * 'RawSyntax' now has - Unowned pointer to the 'SyntaxArena' - Tagged union of "token" or "layout" where all data are managed by the 'SyntaxArena' * Root 'SyntaxData' now 'SyntaxArena' of the root 'RawSyntax'

CodaFi

You love to see it.

rintaro · 2022-08-05T23:23:10Z

@swift-ci Please test

rintaro · 2022-08-06T04:43:47Z

The test error is:

/home/build-user/swift-syntax/lit_tests/coloring.swift:235:12: error: CHECK: expected string not found in input
 // CHECK: <str>"</str>\<anchor>(</anchor><int>1</int><anchor>)</anchor><str></str>\<anchor>(</anchor><int>1</int><anchor>)</anchor><str>"</str>
           ^
<stdin>:148:11: note: scanning from here
 """</str>
          ^
<stdin>:150:2: note: possible intended match here
 <str>"</str>\<anchor>(</anchor><int>1</int><anchor>)</anchor>\<anchor>(</anchor><int>1</int><anchor>)</anchor><str>"</str>
 ^

The difference is <str></str> missing in the actual result, but I feel the new behavior is preferable. What do you think @ahoppen ?

NOTE: This happens because I removed the stored "source presence" flag from RawSyntax, and tokens with empty string is now considered .missing. So the classifier skips the empty token.

rintaro · 2022-08-06T04:46:45Z

@swift-ci Please test

rintaro · 2022-08-06T06:20:17Z

@swift-ci Please test

ahoppen

Overall, looks really good to me. I’ve got a few comments, mostly regarding doc comments, inline. Some general comments:

I’m not sure we should use RawTriviaPiece.make instead of an initializer. But we can discuss this as a follow-up.
Looking through this, I’m not sure if you’re always consistent whether SyntaxArena is the first or last parameter. I think we should be consistent in this.
Should the preconditions be assertions instead that are disabled in Release builds?
Would it make sense to group methods on RawSyntax into three different categories (which are different extensions, separated by a // MARK: - ):
- Methods that work on all RawSyntax nodes
- Methods that fail a precondition when invoked on tokens
- Methods that fail a precondition when invoked on layouts

And if you make any changes to this PR, could you make them in separate commits. That would make reviewing of these changes a lot easier because I don’t have to look through all the other changes again. And then you can squash once we agree this PR is good to merge.

Sources/SwiftSyntaxParser/SyntaxParser.swift

Sources/SwiftSyntax/Trivia.swift.gyb

Sources/SwiftSyntax/TokenKind.swift.gyb

Sources/SwiftSyntax/SyntaxData.swift

Sources/SwiftSyntax/RawSyntax.swift

ahoppen · 2022-08-08T13:31:27Z

Sources/SwiftSyntax/RawSyntax.swift

+    // Allocate and initialize the list.
+    let layoutBuffer = arena.allocateRawSyntaxBuffer(count: count)
+    initializer(layoutBuffer)
+    // validateLayout(layout: RawSyntaxBuffer(layoutBuffer), as: kind)


Do you intend to add the validation again or are we dropping it?

Adding. To be precise, I'm moving the validation from Syntax to RawSyntax.
https://github.com/rintaro/swift-syntax/blob/bump-ptr-allocation-and-everything/Sources/SwiftSyntax/gyb_generated/SyntaxValidation.swift
And do it whenever it's created.

OK, I’m fine with adding validation again in a follow-up PR.

Sources/SwiftSyntax/Trivia.swift.gyb

rintaro · 2022-08-08T20:14:51Z

Looking through this, I’m not sure if you’re always consistent whether SyntaxArena is the first or last parameter. I think we should be consistent in this.

The current rule is:

Factories (e.g. makeXXX(), usually static method) receives the SyntaxArena first
Modifiers (e.g. withXXX(), appendingXXX(), instance methods) receives the SyntaxArena last

I think arena should be first in general, but modifiers receiving it first (e.g. replacingChild(arena: arena, at: index, with: newChild)) doesn't look natural.

rintaro · 2022-08-08T20:24:37Z

Should the preconditions be assertions instead that are disabled in Release builds?

I only used precondition for some RawSyntax modifiers, and a couple of SyntaxArena methods where I think the performance penalty is acceptable, and it shouldn't really happen.
We could use assert for RawSyntax modifiers as it is not public, but I have a plan to expose it as a SPI, so I'd like to keep them precondition.

rintaro · 2022-08-08T20:27:59Z

Would it make sense to group methods on RawSyntax into three different categories

Yes totally. I think we could even split them into separate files. But I just tried to keep the existing code/method order to keep this PR as small as possible.

- arena: SyntaxArena goes the last - Remove RawSyntax: Identifiable

rintaro · 2022-08-08T22:32:41Z

apple/swift-stress-tester#183
@swift-ci Please test

rintaro · 2022-08-08T23:21:06Z

apple/swift-stress-tester#183
@swift-ci Please test

This reverts commit 2cb6af7.

rintaro · 2022-08-09T00:26:43Z

apple/swift-stress-tester#183
@swift-ci Please test

rintaro · 2022-08-09T05:41:38Z

apple/swift-stress-tester#183
@swift-ci Please test

references.

rintaro · 2022-08-09T07:06:03Z

apple/swift-stress-tester#183
@swift-ci Please test

rintaro requested a review from ahoppen as a code owner August 2, 2022 23:03

rintaro changed the title ~~Bump allocated RawSyntax~~ [WIP]Bump allocated RawSyntax Aug 2, 2022

CodaFi reviewed Aug 5, 2022

View reviewed changes

rintaro force-pushed the rewrite-rawsyntax branch 3 times, most recently from 219743b to 6511704 Compare August 5, 2022 22:13

CodaFi approved these changes Aug 5, 2022

View reviewed changes

rintaro force-pushed the rewrite-rawsyntax branch 2 times, most recently from 1c41cae to c08a749 Compare August 5, 2022 22:44

wip

95daf90

rintaro force-pushed the rewrite-rawsyntax branch from c08a749 to 95daf90 Compare August 5, 2022 23:10

wip

92588d3

text fix

0a90fea

fix

0c014dc

ahoppen reviewed Aug 8, 2022

View reviewed changes

rintaro added 2 commits August 8, 2022 11:43

Review update apple#1

bb38b33

remove .parentBox

a59576d

ahoppen approved these changes Aug 8, 2022

View reviewed changes

rintaro added 3 commits August 8, 2022 15:15

Review fix

ba27d00

- arena: SyntaxArena goes the last - Remove RawSyntax: Identifiable

Remvoe null termination for source buffer

2cb6af7

Comment-out deprecation for SyntaxArena.default

b746d21

Update a comment

d215ae2

rintaro added 2 commits August 8, 2022 17:21

revert test

9bf04fd

Revert "Remvoe null termination for source buffer"

b230b79

This reverts commit 2cb6af7.

rintaro mentioned this pull request Aug 9, 2022

Update RequestActionGenerator test case for SwiftSyntax changes apple/swift-stress-tester#183

Merged

Don't use raw.arena to replace the layout. It causes cyclic arena

57fa10d

references.

rintaro force-pushed the rewrite-rawsyntax branch from a3e87ba to 57fa10d Compare August 9, 2022 07:05

rintaro merged commit 342a9d2 into apple:main Aug 9, 2022

rintaro changed the title ~~[WIP]Bump allocated RawSyntax~~ Bump allocated RawSyntax Aug 9, 2022

akyrtzi mentioned this pull request Aug 24, 2022

Use RawSyntax address as the 'rootId' of SyntaxIdentifier #635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump allocated RawSyntax #544

Bump allocated RawSyntax #544

rintaro commented Aug 2, 2022 •

edited

CodaFi Aug 5, 2022

CodaFi Aug 5, 2022

CodaFi left a comment

rintaro commented Aug 5, 2022

rintaro commented Aug 6, 2022 •

edited

rintaro commented Aug 6, 2022

rintaro commented Aug 6, 2022

ahoppen left a comment

ahoppen Aug 8, 2022

rintaro Aug 8, 2022

ahoppen Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 9, 2022

rintaro commented Aug 9, 2022

rintaro commented Aug 9, 2022

		@@ -0,0 +1,113 @@
		//===------------------ RawSyntax.swift - Raw Syntax nodes ----------------===//

Bump allocated RawSyntax #544

Bump allocated RawSyntax #544

Conversation

rintaro commented Aug 2, 2022 • edited

CodaFi Aug 5, 2022

Choose a reason for hiding this comment

CodaFi Aug 5, 2022

Choose a reason for hiding this comment

CodaFi left a comment

Choose a reason for hiding this comment

rintaro commented Aug 5, 2022

rintaro commented Aug 6, 2022 • edited

rintaro commented Aug 6, 2022

rintaro commented Aug 6, 2022

ahoppen left a comment

Choose a reason for hiding this comment

ahoppen Aug 8, 2022

Choose a reason for hiding this comment

rintaro Aug 8, 2022

Choose a reason for hiding this comment

ahoppen Aug 8, 2022

Choose a reason for hiding this comment

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 8, 2022

rintaro commented Aug 9, 2022

rintaro commented Aug 9, 2022

rintaro commented Aug 9, 2022

rintaro commented Aug 2, 2022 •

edited

rintaro commented Aug 6, 2022 •

edited