Increase the amount of recursive parsing calls for statements+blocks. #41639
Conversation
|
Tagging @RikkiGibson @jcouv . WIth this change (and my other PRs) we no longer have any regression in nested-statement-parsing (even when supporting attributes on all statements). |
CyrusNajmabadi
force-pushed
the
blockStackFrames
branch
from
b687958 to
53cb76e
Compare
|
Ping jcouv on this one as well. |
|
|
||
| // There's a special error code for a missing token after an accessor keyword | ||
| var openBrace = isAccessorBody && this.CurrentToken.Kind != SyntaxKind.OpenBraceToken | ||
| CSharpSyntaxNode openBrace = isAccessorBody && this.CurrentToken.Kind != SyntaxKind.OpenBraceToken |
There was a problem hiding this comment.
is an explicit variable type required here?
There was a problem hiding this comment.
it makes things cleaner. we later on call a method that takes a ref CSharpSyntaxNode. so this avoids having to copy this SyntaxToken into a CSharpSyntaxNode and back again.
| { | ||
| _pool.Free(statements); | ||
| } | ||
| _pool.Free(statements); |
There was a problem hiding this comment.
good call on removing the try/finally, that by itself can increase stack frame size and prevent inlining.
I am assuming that when we're in a situation that we throw an exception while parsing the block, that presents as a non-recoverable error in the IDE? So we don't have to worry about carrying on while leaking memory?
There was a problem hiding this comment.
Correct.
Note: we dont' actually 'leak' in that case. We merely don't get as much pooled reuse of these data structures as we woudl otherwise. However, that's really not important enough to impact a more relevant scenario like "how much can we parse".
The major downside here is that you have to be more careful to not 'early return'. So i only do this sort of thing when the code is simple enough that it's easy to visually validate that the pooled resources are returned.
|
|
||
| var block = _syntaxFactory.Block( | ||
| (SyntaxToken)openBrace, | ||
| statements, |
There was a problem hiding this comment.
this is so confusing. it makes it look like you're creating the block, then freeing something used to create it, then returning the block. i.e. a bad bug. I would really like to see this conversion removed (SyntaxListBuilder->SyntaxList)
There was a problem hiding this comment.
happy to do that in a future PR. i also do not like the implicit conversion from builder to list.
RikkiGibson
added
the
Community
ghost
merged commit
|
Thanks! |
3 participants
Before this, we can parse 1333 nested if-statements. Afterwards, we get up to 1443. This is a simple increase of >8%.
We do this by breaking out block-parsing into two separate helpers. One used for method/accessor blocks and one for normal statement-blocks.
This allows us to keep all specialized parsing/stack-space for the method case, while keeping the statement-version extremely simple and lightweight.