Skip to content

Conversation

@am11
Copy link
Member

@am11 am11 commented Jun 29, 2022

First commit updates toolset version and applies @AaronRobinsonMSFT's lifetime patch from dotnet/installer#14029.

Second commit uses u8 string for CultureNames, and updates its generator. File size of IcuLocaleData.cs goes from 277740 to 227081 (bytes).

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Globalization labels Jun 29, 2022
@ghost
Copy link

ghost commented Jun 29, 2022

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

First commit updates toolset version and applies @AaronRobinsonMSFT's lifetime patch from dotnet/installer#14029.

Second commit uses u8 string for CultureNames, and updates its generator. File size of IcuLocaleData.cs goes from 277740 to 247866 (bytes).

Author: am11
Assignees: -
Labels:

area-System.Globalization, community-contribution

Milestone: -

@am11 am11 requested a review from stephentoub June 29, 2022 04:07
@am11 am11 changed the title Use u8 in CultureNames Use u8 string in CultureNames Jun 29, 2022
@AaronRobinsonMSFT
Copy link
Member

@am11 Those changes are really a point in time and shouldn't be checked into main. Moving to a new compiler with ref field support should be paired with the merging in of https://github.com/dotnet/runtime/tree/feature/csharp_ref_fields.

@am11
Copy link
Member Author

am11 commented Jun 29, 2022

@AaronRobinsonMSFT, could you please rebase that branch current main? I can then merge it here.

@stephentoub
Copy link
Member

Let's please wait to do this PR until Aaron's changes that bring in a new compiler have all landed. There's no rush on this PR as it's purely maintainability. Thanks.

@stephentoub stephentoub marked this pull request as draft June 29, 2022 10:58
@AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT, could you please rebase that branch current main? I can then merge it here.

I should be able to get that in a PR today or tomorrow.

@tarekgh tarekgh added this to the 7.0.0 milestone Jun 29, 2022
@am11 am11 force-pushed the feature/u8-strings branch from 42170fe to 74f14c7 Compare July 3, 2022 12:53
@am11
Copy link
Member Author

am11 commented Jul 3, 2022

Compiler is crashing with Stack Overflow (on Windows and Applie's OSes):

/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error : Stack overflow. [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error : Repeat 339 times: [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error : -------------------------------- [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.CheckValEscape(Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.CSharp.BoundExpression, UInt32, UInt32, Boolean, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error : -------------------------------- [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.ValidateEscape(Microsoft.CodeAnalysis.CSharp.BoundExpression, UInt32, Boolean, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.<BindExpressionBodyAsBlock>g__bindExpressionBodyAsBlockInternal|960_0(Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax, Microsoft.CodeAnalysis.CSharp.Binder, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionBodyAsBlock(Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.Binder.BindMethodBody(Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.MethodCompiler.BindMethodBody(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol, Microsoft.CodeAnalysis.CSharp.TypeCompilationState, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Boolean, Microsoft.CodeAnalysis.CSharp.BoundNode, Boolean, Microsoft.CodeAnalysis.CSharp.ImportChain ByRef, Boolean ByRef, Boolean ByRef, InitialState ByRef) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethod(Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol, Int32, ProcessedFieldInitializers ByRef, Microsoft.CodeAnalysis.CSharp.SynthesizedSubmissionFields, Microsoft.CodeAnalysis.CSharp.TypeCompilationState) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedType(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Microsoft.CodeAnalysis.CSharp.MethodCompiler+<>c__DisplayClass25_0.<CompileNamedTypeAsync>b__0() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at Roslyn.Utilities.UICultureUtilities+<>c__DisplayClass5_0.<WithCurrentUICulture>b__0() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(System.Threading.Thread, System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at System.Threading.Tasks.Task.ExecuteWithThreadLocal(System.Threading.Tasks.Task ByRef, System.Threading.Thread) [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at System.Threading.ThreadPoolWorkQueue.Dispatch() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]
/Users/runner/work/1/s/.packages/microsoft.net.compilers.toolset/4.4.0-1.22328.22/tasks/net6.0/Microsoft.CSharp.Core.targets(75,5): error :    at System.Threading.Thread.StartCallback() [/Users/runner/work/1/s/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj]

@AlekseyTs, is it a known issue?

@am11
Copy link
Member Author

am11 commented Jul 3, 2022

@AlekseyTs, is it a known issue?

Reported dotnet/roslyn#62361 with additional info in case it wasn't.

@jcouv
Copy link
Member

jcouv commented Jul 8, 2022

Reported dotnet/roslyn#62361 with additional info in case it wasn't.

The roslyn issue was fixed. Updated roslyn packages are usually available within a day on the internal feeds (dotnet-tools) and on the public feeds once per preview.

@stephentoub stephentoub force-pushed the feature/u8-strings branch from 74f14c7 to 89d9151 Compare July 8, 2022 16:26
@stephentoub stephentoub marked this pull request as ready for review July 8, 2022 16:27
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

@stephentoub
Copy link
Member

Looks there still a problem?

Yup. @jcouv do you know what min version has the fix?

@am11
Copy link
Member Author

am11 commented Jul 8, 2022

I think we would need to wait for the next v4.4.0-x to pick up the change. (the subsequent automation PR after dotnet/roslyn#62492 is merged or abandoned).

@jcouv
Copy link
Member

jcouv commented Jul 8, 2022

@jcouv do you know what min version has the fix?

Not precisely, but the fix was merged only 2 hours ago. I'd suggest checking the internal feed in a day or two.

@stephentoub
Copy link
Member

Gotcha, I read your previous note too quickly and thought it was fixed before today. We'll wait :-)

@stephentoub
Copy link
Member

Thanks!

@stephentoub stephentoub merged commit d344e72 into dotnet:main Jul 9, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Globalization community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants