Skip to content

Use syntaxtrees in incremental processing to avoid unnecessary work#62071

Merged
CyrusNajmabadi merged 8 commits intodotnet:mainfrom
CyrusNajmabadi:syntaxTreeIncremental
Jun 27, 2022
Merged

Use syntaxtrees in incremental processing to avoid unnecessary work#62071
CyrusNajmabadi merged 8 commits intodotnet:mainfrom
CyrusNajmabadi:syntaxTreeIncremental

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Jun 22, 2022

This drops the time of the benchmark from 15 seconds to 10, and drops allocations from 15GB to 12.5GB.

Above data was for only updating one call. Updating both drops us from 15 seconds to 3.3, and from 15B to 4.7GB

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners June 22, 2022 03:22
@ghost ghost added the Area-Compilers label Jun 22, 2022

var syntaxTreesProvider = _context.CompilationProvider.SelectMany((c, _) => c.SyntaxTrees);
var individualFileGlobalAliasesProvider = syntaxTreesProvider.Select(
(s, c) => getGlobalAliasesInCompilationUnit(syntaxHelper, s.GetRoot(c))).WithTrackingName("individualFileGlobalAliases_ForAttribute");
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jaredpar @chsienki it's unclear to me if we'd actually need the 'pure syntax provdier' given that we can just simulate it by getting the syntax-trees from teh compilation-provider. these are already pure-syntax, and already support the equality semantics we need here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge June 22, 2022 05:40
@CyrusNajmabadi CyrusNajmabadi disabled auto-merge June 22, 2022 06:08
static (n, _) => n is ICompilationUnitSyntax,
static (context, _) => context.Node).WithTrackingName("compilationUnit_ForAttribute");
var compilationUnitProvider = syntaxTreesProvider.Select(
(t, c) => t.GetRoot(c)).WithTrackingName("compilationUnit_ForAttribute");
Copy link
Copy Markdown
Member

@jcouv jcouv Jun 22, 2022

Choose a reason for hiding this comment

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

static?

Suggested change
(t, c) => t.GetRoot(c)).WithTrackingName("compilationUnit_ForAttribute");
static (t, c) => t.GetRoot(c)).WithTrackingName("compilationUnit_ForAttribute");
``` #Closed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually, noticing this, this is bad. we don't want to hold onto the roots of all the trees (as that will prevent them from being GC'ed). moving this to only hold the syntax tree, and to delay getting the root until necessary.

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 5) with a nit

@jcouv jcouv self-assigned this Jun 22, 2022
@CyrusNajmabadi CyrusNajmabadi enabled auto-merge June 22, 2022 18:24
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 8)

@CyrusNajmabadi CyrusNajmabadi disabled auto-merge June 22, 2022 22:29
@CyrusNajmabadi CyrusNajmabadi enabled auto-merge June 22, 2022 22:29
@jcouv jcouv disabled auto-merge June 27, 2022 16:45
@CyrusNajmabadi CyrusNajmabadi merged commit 737ef1b into dotnet:main Jun 27, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the syntaxTreeIncremental branch June 27, 2022 17:25
@ghost ghost added this to the Next milestone Jun 27, 2022
@RikkiGibson RikkiGibson removed this from the Next milestone Jun 28, 2022
@RikkiGibson RikkiGibson added this to the 17.3 P3 milestone Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants