{WIP} (GH-813) Use AST for Syntax Folder #806
Conversation
glennsarti
force-pushed
the
spike-try-ast
branch
from
0e2dc4c to
f17d929
Compare
|
Because the AST drops comments we need to use a combination of AST and Tokens. The AST+tokens method now has parity in features with the tokens only method. Initial performance tests; Scenario The same script is give to both folders. It should contain 4096 regions. This is a VERY LARGE number and is considered an edge case. However the large number should show differences easier. The script is passed through both folders 20 times, but the first result is ignored. Due to C# internal "stuff" the first time it runs the duration is 130+ms but subsequent runs are < 40ms Results
Raw Data; |
|
So after some playing around. Enumerating over tokens is relatively cheap ~1-2ms. However running regexs is where the processing time is being taken up. After merging the three comment extraction methods into one, I could remove one of the three regexes. In the performance example I'm using, I managed to shave off 8ms. Bringing the total to 10ms. |
|
I tried using a hashtable instead of an array to hold the foldable regions but it only gained 1ms at best. |
glennsarti
force-pushed
the
spike-try-ast
branch
2 times, most recently
from
a6f9341 to
22ead9d
Compare
glennsarti
changed the title
glennsarti
force-pushed
the
spike-try-ast
branch
2 times, most recently
from
b24e7c4 to
ffe2485
Compare
glennsarti
changed the title
|
Rebased. Ready for merge ping @rjmholt |
rjmholt
requested review from
SeeminglyScience,
TylerLeonhardt and
rkeithhill
test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs
Outdated
Show resolved
Hide resolved
glennsarti
force-pushed
the
spike-try-ast
branch
from
ffe2485 to
e33ba46
Compare
|
@rjmholt Managed to fix most of your requests. Only 2 or 3 outstanding questions. I think your suggestions will also have had a minor speed improvement too. Haven't taken the time to prove it yet. |
|
Bouncing the PR due to transient Appveyor failure in |
|
CI is green ready for review. |
| { | ||
| tokenCommentRegionStack.Push(token); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I would put a newline here and in places below where new condition blocks begin
There was a problem hiding this comment.
I'm not convinced on this. I kept them grouped because they are all to do with # region detection. I didn't refactor to a method because I merged two functions together to create this. Otherwise I end up doing too many regexes and comparions.
glennsarti
changed the title
glennsarti
force-pushed
the
spike-try-ast
branch
from
e2dc02b to
0cd9671
Compare
|
Bouncing the PR to force appveyor to re-run |
|
Bouncing the PR to force appveyor to re-run |
| int endLineOffset = 0; | ||
| // If we're showing the last line, decrement the Endline of all regions by one. | ||
| if (this.currentSettings.CodeFolding.ShowLastLine) { endLineOffset = -1; } | ||
| foreach (FoldingReference fold in FoldingOperations.FoldableRegions( |
There was a problem hiding this comment.
Instead of a line break in the foreach, you can also save it to a local variable first. That'll be optimized out by the compiler in a release build.
There was a problem hiding this comment.
Not quite sure what you mean here.... do you mean;
var temp = FoldingOperations.FoldableRegions(script.ScriptTokens, script.ScriptAst)
foreach (FoldingReference fold in temp)
{
....
If so what is that actually getting us?
| /// <summary> | ||
| /// The visitor used to find the all folding regions in an AST | ||
| /// </summary> | ||
| internal class FindFoldsVisitor : AstVisitor |
There was a problem hiding this comment.
Sorry if I sound like a broken record, but we're sure this works with classes right? I would have expected this to either need to inherit AstVisitor2 or dip into tokens for classes. Can we get a basic class example added to the tests? Something like
class TestClass {
[string[]] $TestProperty = @(
'first',
'second',
'third')
[string] TestMethod() {
return $this.TestProperty[0]
}
}There was a problem hiding this comment.
Welp...this is a problem....works on master but not after this PR.... More work for me to do.
There was a problem hiding this comment.
So we did indeed need to use ASTVisitor2 for the VisitTypeDefinition method. Added a test for this and is now passing.
Fixed.
The AST contains the most correct version of how a script is interpreted. This includes regions of text. Currently the code folder only uses the Tokens which requires the folder to re-implement some of the AST behaviour e.g. matching token pairs for arrays etc. The code folder should be implemented using as much of the AST as possible. This commit; * Moves most of the region detection to use the AST Extents and uses a new FindFoldsASTVisitor. * Modifies the tests and language server to use the new method fold detection class. * Moved the code to modify the end line of folding regions to the language server code.
…o it's own class Previously the folding provider created many intermediate arrays and lists and required post-processing. This commit changes the behaviour to use an accumlator patter with an extended Dictionary class. This new class adds a `SafeAdd` method to add FoldingRanges, which then has the logic to determine if the range should indeed be added, for example, passing nulls or pre-existing larger ranges. By passing around this list using ByReference we can avoid creating many objects which are just then thrown away. This commit also moves the ShowLastLine code from the FoldingProvider into the Language Server. This reduces the number of array enumerations to one.
glennsarti
force-pushed
the
spike-try-ast
branch
from
0cd9671 to
e4744ca
Compare
|
We can't use the AST as it's heavily dependant on PS version. e.g. I need to use the latest AST Visitor but it doesn't exist in PS3. This means I need to use tokens, not the AST. I'll raise a new PR with a faster token processor. |
|
If you would like, you could send this over to the |
|
@TylerLeonhardt Yeah I need ASTVisitor2 :-( |
|
Retargetting to 2.0.0... I expect a WHOLE bunch of issues till I rebase. |
|
Yeah let us rebase 2.0.0 to master. That will help |
|
Closed in preference to #853 |
Requires #825 to be merged first and that merged back to the 2.0.0 branch
DO NOT MERGE
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc. The code folder should be implemented using as much
of the AST as possible. This commit;
FindFoldsASTVisitor.
class.
Managed to go from 32ms to 20ms