Skip to content

Increase the amount of recursive parsing calls for statements+blocks. #41639

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.

Sign up for GitHub

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
3 commits merged into from
Feb 13, 2020
Merged

Increase the amount of recursive parsing calls for statements+blocks. #41639

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53cb76e
Increase the amount of recursive parsing calls for statements+blocks.
CyrusNajmabadi Feb 13, 2020
6883b87
Simplify
CyrusNajmabadi Feb 13, 2020
29793af
Remove comment
CyrusNajmabadi Feb 13, 2020
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
105 changes: 49 additions & 56 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2450,7 +2450,7 @@ private void ParseBlockAndExpressionBodiesWithSemicolon(

if (this.CurrentToken.Kind == SyntaxKind.OpenBraceToken)
{
blockBody = this.ParseBlock(isMethodBody: true);
blockBody = this.ParseMethodOrAccessorBodyBlock(isAccessorBody: false);
}

if (this.CurrentToken.Kind == SyntaxKind.EqualsGreaterThanToken)
Expand All @@ -2477,25 +2477,6 @@ private void ParseBlockAndExpressionBodiesWithSemicolon(
}
}

private void ParseBodyOrSemicolon(out BlockSyntax body, out SyntaxToken semicolon)
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Feb 13, 2020

Choose a reason for hiding this comment

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

unused. deleted.

{
if (this.CurrentToken.Kind == SyntaxKind.OpenBraceToken)
{
body = this.ParseBlock(isMethodBody: true);

semicolon = null;
if (this.CurrentToken.Kind == SyntaxKind.SemicolonToken)
{
semicolon = this.EatTokenWithPrejudice(ErrorCode.ERR_UnexpectedSemicolon);
}
}
else
{
semicolon = this.EatToken(SyntaxKind.SemicolonToken);
body = null;
}
}

private bool IsEndOfTypeParameterList()
{
if (this.CurrentToken.Kind == SyntaxKind.OpenParenToken)
Expand Down Expand Up @@ -3305,7 +3286,7 @@ private AccessorDeclarationSyntax ParseAccessorDeclaration(bool isEvent)
{
if (!IsTerminator())
{
blockBody = this.ParseBlock(isMethodBody: true, isAccessorBody: true);
blockBody = this.ParseMethodOrAccessorBodyBlock(isAccessorBody: true);
}
else
{
Expand Down Expand Up @@ -6941,54 +6922,66 @@ private bool IsPossibleNewExpression()
return null;
}

// If "isMethodBody" is true, then this is the immediate body of a method/accessor.
// In this case, we create a many-child list if the body is not a small single statement.
// This then allows a "with many weak children" red node when the red node is created.
// If "isAccessorBody" is true, then we produce a special diagnostic if the open brace is
// missing. Also, "isMethodBody" must be true.
private BlockSyntax ParseBlock(bool isMethodBody = false, bool isAccessorBody = false)
/// <summary>
/// Used to parse the block-body for a method or accessor. For blocks that appear *inside*
/// method bodies, call <see cref="ParseBlock"/>.
/// </summary>
/// <param name="isAccessorBody">If is true, then we produce a special diagnostic if the
/// open brace is missing.</param>
private BlockSyntax ParseMethodOrAccessorBodyBlock(bool isAccessorBody)
{
// Check again for incremental re-use, since ParseBlock is called from a bunch of places
// other than ParseStatementCore()
// Check again for incremental re-use. This way if a method signature is edited we can
// still quickly re-sync on the body.
if (this.IsIncrementalAndFactoryContextMatches && this.CurrentNodeKind == SyntaxKind.Block)
{
return (BlockSyntax)this.EatNode();
}

// 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
Copy link
Member

@RikkiGibson RikkiGibson Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

is an explicit variable type required here?

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Feb 13, 2020

Choose a reason for hiding this comment

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

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.

? this.AddError(
SyntaxFactory.MissingToken(SyntaxKind.OpenBraceToken),
IsFeatureEnabled(MessageID.IDS_FeatureExpressionBodiedAccessor)
? ErrorCode.ERR_SemiOrLBraceOrArrowExpected
: ErrorCode.ERR_SemiOrLBraceExpected)
? ErrorCode.ERR_SemiOrLBraceOrArrowExpected
: ErrorCode.ERR_SemiOrLBraceExpected)
: this.EatToken(SyntaxKind.OpenBraceToken);

var statements = _pool.Allocate<StatementSyntax>();
try
{
CSharpSyntaxNode tmp = openBrace;
this.ParseStatements(ref tmp, statements, stopOnSwitchSections: false);
openBrace = (SyntaxToken)tmp;
var closeBrace = this.EatToken(SyntaxKind.CloseBraceToken);
this.ParseStatements(ref openBrace, statements, stopOnSwitchSections: false);

SyntaxList<StatementSyntax> statementList;
if (isMethodBody && IsLargeEnoughNonEmptyStatementList(statements))
{
// Force creation a many-children list, even if only 1, 2, or 3 elements in the statement list.
statementList = new SyntaxList<StatementSyntax>(SyntaxList.List(((SyntaxListBuilder)statements).ToArray()));
}
else
{
statementList = statements;
}
var block = _syntaxFactory.Block(
(SyntaxToken)openBrace,
// Force creation a many-children list, even if only 1, 2, or 3 elements in the statement list.
IsLargeEnoughNonEmptyStatementList(statements)
? new SyntaxList<StatementSyntax>(SyntaxList.List(((SyntaxListBuilder)statements).ToArray()))
: statements,
this.EatToken(SyntaxKind.CloseBraceToken));

return _syntaxFactory.Block(openBrace, statementList, closeBrace);
}
finally
{
_pool.Free(statements);
}
_pool.Free(statements);
Copy link
Member

@RikkiGibson RikkiGibson Feb 13, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Feb 13, 2020

Choose a reason for hiding this comment

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

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.

return block;
}

/// <summary>
/// Used to parse normal blocks that appear inside method bodies. For the top level block
/// of a method/accessor use <see cref="ParseMethodOrAccessorBodyBlock"/>.
/// </summary>
private BlockSyntax ParseBlock()
{
// Check again for incremental re-use, since ParseBlock is called from a bunch of places
// other than ParseStatementCore()
if (this.IsIncrementalAndFactoryContextMatches && this.CurrentNodeKind == SyntaxKind.Block)
return (BlockSyntax)this.EatNode();

CSharpSyntaxNode openBrace = this.EatToken(SyntaxKind.OpenBraceToken);

var statements = _pool.Allocate<StatementSyntax>();
this.ParseStatements(ref openBrace, statements, stopOnSwitchSections: false);

var block = _syntaxFactory.Block(
(SyntaxToken)openBrace,
statements,
Copy link
Member

@RikkiGibson RikkiGibson Feb 13, 2020

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Feb 13, 2020

Choose a reason for hiding this comment

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

happy to do that in a future PR. i also do not like the implicit conversion from builder to list.

this.EatToken(SyntaxKind.CloseBraceToken));

_pool.Free(statements);
return block;
}

// Is this statement list non-empty, and large enough to make using weak children beneficial?
Expand Down