Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Feb 11, 2020

This reduces teh amount of stack frames we need when parsing. It should actually routinely halve the number as there's no need for all the indirections.

This should be reviewed one commit at a time to make the changes clear. Individual commits are lkely clearer with whitespace diffs off.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 11, 2020 21:48
@CyrusNajmabadi
Copy link
Member Author

Tagging @RikkiGibson

Copy link
Member Author

Choose a reason for hiding this comment

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

this removes the need for one layer of statement indirection. instead of having the no-local-decl statement parsing helper, we just have the single ParseStatemnetCore that the caller can direct the behavior of.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: only the script-parsin code ever gets into this path. It is the only one that doesn't have top level decls, but instead parses out fields. I don't actually love that thsi is how it works. I think it would actually be cleaner to parse out local-decls and then just resynthesize field decls from them. but i think that could be done in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

we still have the main dispatch switch from before.

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 just inlined this as it was the only usage of this helper.

Copy link
Member Author

Choose a reason for hiding this comment

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

'const' handling is already handled by IsPossibleLocalDeclaration below.

@CyrusNajmabadi
Copy link
Member Author

@RikkiGibson do you want to test out hte stack-frame stuff here to see if this improves things enough?

@CyrusNajmabadi
Copy link
Member Author

@RikkiGibson EndToEndTests.NestedIfStatements passes on my machine. Can you send me instructions on capturing stack frame size?

ghost
ghost previously approved these changes Feb 13, 2020
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@RikkiGibson
Copy link
Member

Oops, applied the tag to the wrong PR.

@RikkiGibson
Copy link
Member

Surprised to see NestedIfStatements failing in release mode here. x86 is down 1650 -> 1576 and x64 is down 780 -> 700.

@RikkiGibson RikkiGibson dismissed ghost ’s stale review February 13, 2020 00:15

invalid

@RikkiGibson
Copy link
Member

@CyrusNajmabadi this PR still has a stack depth regression in it.

@RikkiGibson RikkiGibson added Blocked Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Feb 13, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

want want [](start = 89, length = 9)

Typo

@RikkiGibson
Copy link
Member

Will take a look once #41639 is merged.

@RikkiGibson
Copy link
Member

The test ParserErrorMessageTests.TooDeepDelegateDeclaration is failing on macOS. Weird.

Assert.Equal() Failure
Expected: 1
Actual:   30000

@CyrusNajmabadi
Copy link
Member Author

The test ParserErrorMessageTests.TooDeepDelegateDeclaration is failing on macOS. Weird.

I think that's because we can now parse this properly (because we've won so much back in the stack) :)

I'll have to up this value to something that will definitely start failing.

@CyrusNajmabadi
Copy link
Member Author

Might be easier to review with whitespace changes off.

@jcouv jcouv added this to the 16.6.P1 milestone Feb 14, 2020
@CyrusNajmabadi
Copy link
Member Author

@cston for visibility as well. Note: i can try to break this up into smaller PRs if that would be helpful. But i've gotten feedback that it would be nice to get this all finished (and this is hte last PR i was intending on making).

@CyrusNajmabadi
Copy link
Member Author

Tagging @jcouv let me know if anything in the PR needs more explanation. I'll try to go through to document what change is going on here since hte diff doesn't look great (either in normal or split view), but the change is conceptually really simple.

Copy link
Member Author

Choose a reason for hiding this comment

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

the allowAnyExpression param name is not very helpful or intuitive as to what it is trying to convey. really, it's only purpose is to say "we're at the global level, parse accordingly".

@CyrusNajmabadi
Copy link
Member Author

I'm going to try to rework this PR into very easy to follow individual commits.

@CyrusNajmabadi
Copy link
Member Author

@jcouv I have rewritten this PR into a series of commits to make this much easier to understand. If you go commit by commit it shuld be much easier.

@RikkiGibson
Copy link
Member

@jcouv FYI, I have verified via #41725 that this PR fixes the stack depth issue in the LFA branch.

@RikkiGibson
Copy link
Member

@jaredpar @dotnet/roslyn-compiler this change is required for local-function-attributes. It would be very helpful to get a second reviewer.

@RikkiGibson RikkiGibson merged commit 99ec12e into dotnet:master Feb 19, 2020
@CyrusNajmabadi CyrusNajmabadi deleted the inlineStatementParsing branch March 2, 2020 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants