Skip to content
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

Jump to bottom

Remove ItemCollection from CodeRenderingContext #10764

Merged
merged 24 commits into from
Aug 22, 2024
Merged

Remove ItemCollection from CodeRenderingContext #10764

merged 24 commits into from
Aug 22, 2024

Conversation

DustinCampbell
Copy link
Member

@DustinCampbell DustinCampbell commented Aug 19, 2024 •
edited
Loading

Because of a dare from @333fred, I'm currently on a crusade to remove ItemCollection from the Razor Compiler completely. Previously, I removed ItemCollection from TagHelperDescriptorProviderContext (#10720). This time, I'm removing it from CodeRenderingContext.

It turns out that every CodeRenderingContext greedily creates an ItemCollection, though there are only ever two things stored there:

  1. A string to use for new lines in CodeWriter.
  2. A string representing "suppress IDs", which is already specified in RazorCodeGenerationOptions.

These two items were really present as a hook that compiler tests could set. However, it's much easier and less wasteful to elevate both items to RazorCodeGenerationOptions and make tests set the options directly.

I made a few other refactorings as part of this change:

  • I merged several abstract base classes with their single default concrete type:
    • CodeRenderingContext and DefaultCodeRenderingContext
    • RazorCSharpDocument and DefaultRazorCSharpDocument,
    • RazorCodeGenerationOptions and DefaultRazorCodeGenerationOptions
    • RazorCodeGenerationOptionsBuilder and DefaultRazorCodeGenerationOptionsBuilder.
  • Reworked RazorCodeGenerationOptions and introduced RazorCodeGenerationOptionsFlags to track its boolean fields.
  • Cleaned up RazorCSharpDocument and unified its collections on ImmutableArray<> rather than IReadOnlyList<>.
  • Enabled nullability annotations for several types.
  • Used more pooled collections in CodeRenderingContext.

Note: I was careful with my commit history, and it should be clean enough to review commit-by-commit if that's your preference.

@DustinCampbell
Merge CodeRenderingContext and DefaultCodeRenderingContext
80d1ed3
@DustinCampbell
Clean up CodeRenderingContext a bit
0e1774a
@DustinCampbell
Pool scope stack in CodeRenderingContext
86e05ee
@DustinCampbell
Pool line pragmas in CodeRenderingContext
635695f
@DustinCampbell
Pool ancestor stack in CodeRenderingContext
7ba4179
@DustinCampbell
Merge RazorCSharpDocument and DefaultRazorCSharpDocument
25acc07
@DustinCampbell
Switch CodeRenderingContext.LinePragmas to an ImmutableArray
d00b1ff
@DustinCampbell
Clean up callers of RazorCSharpDocument.Create(...)
89204d4
@DustinCampbell
Swap RazorCSharpDocument.Create methods for constructors
c755804
@DustinCampbell
Use pooled ImmutableArray builder for CodeRenderingContext diagnostics
ff63a4e
@DustinCampbell
Switch RazorCSharpDocument.Diagnostics to an ImmutableArray
90f90ef
@DustinCampbell
Clean up pooled objects in CodeRenderingContext
5072db4
@DustinCampbell
Avoid extra allocations in IntermediateNodeExtensions.GetAllDiagnostics
34507da
@DustinCampbell
Remove ItemCollection from CodeRenderingContext
7ac711b
@DustinCampbell
Merge RazorCodeGenerationOptionsBuilder and DefaultRazorCodeGeneratio…
03e2d07 
…nOptionsBuilder
@DustinCampbell
Merge RazorCodeGenerationOptions and DefaultRazorCodeGenerationOptions
972691b
@DustinCampbell
Clean up RazorCodeGenerationOptions a bit
dd11c5e
@DustinCampbell DustinCampbell requested review from a team as code owners August 19, 2024 23:47
DustinCampbell
DustinCampbell commented Aug 19, 2024
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.ExternalAccess.LegacyEditor/RazorWrapperFactory.cs
Comment on lines -61 to -80
private static ImmutableArray<TResult> WrapAll<TInner, TResult>(IReadOnlyList<TInner> list, Func<TInner, TResult> createWrapper)
where TInner : class
where TResult : class
{
var count = list.Count;
if (count == 0)
{
return ImmutableArray<TResult>.Empty;
}

using var builder = new PooledArrayBuilder<TResult>(capacity: count);

for (var i = 0; i < count; i++)
{
builder.Add(createWrapper(list[i]));
}

return builder.DrainToImmutable();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

ℹ️ After converting RazorCSharpDocument to an ImmutableArray<RazorDiagnostic>, this method is no longer used.

davidwengier
davidwengier approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@davidwengier davidwengier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tooling ✅

@maryamariyan
Copy link
Member

Used more pooled collections in CodeRenderingContext

is it worth adding concurrency tests to make sure the usage is thread safe?

@DustinCampbell
Copy link
Member Author

Used more pooled collections in CodeRenderingContext

is it worth adding concurrency tests to make sure the usage is thread safe?

Razor compilation is generally synchronous and CodeRenderingContext shouldn't be accessed by more than a single thread. Also, nothing about CodeRenderingContext before this change was more or less thread safe than it is now. Is there a scenario that you're thinking might be problematic? Is there something related to pooled collections that you think might not be thread-safe? Note that acquiring and returning a collection from a pool is an inherently thread-safe operation.

chsienki
chsienki approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

Compiler side LGTM

@maryamariyan
Copy link
Member

Used more pooled collections in CodeRenderingContext

is it worth adding concurrency tests to make sure the usage is thread safe?
Razor compilation is generally synchronous and CodeRenderingContext shouldn't be accessed by more than a single thread. Also, nothing about CodeRenderingContext before this change was more or less thread safe than it is now. Is there a scenario that you're thinking might be problematic? Is there something related to pooled collections that you think might not be thread-safe? Note that acquiring and returning a collection from a pool is an inherently thread-safe operation.

it sounds like we don't need to worry about concurrency. I just wanted to double-check.

@chsienki chsienki mentioned this pull request Aug 22, 2024
Merged
@DustinCampbell DustinCampbell merged commit 3423142 into dotnet:main Aug 22, 2024
12 checks passed
@DustinCampbell DustinCampbell deleted the remove-item-collection2 branch August 22, 2024 20:53
@DustinCampbell
Copy link
Member Author

Thanks all!

@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 22, 2024
@jjonescz jjonescz mentioned this pull request Aug 23, 2024
Merged
@dibarbet dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024
This was referenced Aug 29, 2024
Closed
Closed
@dotnet-bot dotnet-bot mentioned this pull request Sep 14, 2024
Closed
@DustinCampbell DustinCampbell mentioned this pull request Sep 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwengier davidwengier davidwengier approved these changes

@chsienki chsienki chsienki approved these changes

@maryamariyan maryamariyan maryamariyan approved these changes

@333fred 333fred 333fred approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
17.12 P2
Development

Successfully merging this pull request may close these issues.
6 participants
@DustinCampbell @maryamariyan @davidwengier @333fred @chsienki @dibarbet