Add feature flag for emitting string literals into data section as UTF8 #76036
+928
−72
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
jjonescz
force-pushed
the
DataSectionStringLiterals
branch
from
16f5293
to
43aa343
Compare
jjonescz
force-pushed
the
DataSectionStringLiterals
branch
from
43aa343
to
e9e701d
Compare
AlekseyTs
reviewed
Nov 27, 2024
| error CS8103: Combined length of user strings used by the program exceeds allowed limit. Try to decrease use of string literals. | ||
| ``` | ||
|
|
||
| By turning on the feature flag `utf8-string-encoding`, string literals (where possible) are instead emitted as UTF-8 data into a different section of the PE file |
AlekseyTs
reviewed
Nov 27, 2024
|
|
||
| namespace Microsoft.CodeAnalysis.CodeGen; | ||
|
|
||
| internal sealed class DataStringHolder : DefaultTypeDef, Cci.INamespaceTypeDefinition |
AlekseyTs
reviewed
Nov 27, 2024
|
|
||
| public DataStringHolder( | ||
| CommonPEModuleBuilder moduleBuilder, | ||
| string dataHash, |
AlekseyTs
reviewed
Nov 27, 2024
| /// </param> | ||
| /// <returns>The field. This may have been newly created or may be an existing field previously created for the same data and alignment.</returns> | ||
| internal Cci.IFieldReference CreateDataField(ImmutableArray<byte> data, ushort alignment) | ||
| internal Cci.ITypeReference GetOrAddProxyType(int length, ushort alignment) |
There was a problem hiding this comment.
Consider renaming _proxyTypes as well
AlekseyTs
reviewed
Nov 27, 2024
|
|
||
| Debug.Assert(_mappedFields.IsEmpty, "We need to generate unique field names if we are synthesizing more than one."); | ||
|
|
||
| (_, StringField stringField) = _mappedFields.GetOrAdd((data, Alignment: 1), key => |
|
@jjonescz Please add details how do we achieve the goal: what artifacts are getting generated, what APIs are needed and how they are used, what compilation phases/layers are involved and how, etc. Basically, I think we need something like an implementation speclet for the compiler feature. |
AlekseyTs
reviewed
Nov 27, 2024
|
|
||
| fieldsBuilder.Sort(PrivateImplementationDetails.FieldComparer.Instance); | ||
|
|
||
| _orderedSynthesizedFields = fieldsBuilder.ToImmutableAndFree(); |
AlekseyTs
reviewed
Nov 27, 2024
| // Call `<PrivateImplementationDetails>.BytesToString(byte*, int)`. | ||
| bytesToStringHelper ??= GetOrSynthesizeBytesToStringHelper(diagnostics); | ||
| ilBuilder.EmitOpCode(ILOpCode.Call, -1); | ||
| ilBuilder.EmitToken(bytesToStringHelper, null, diagnostics); |
AlekseyTs
reviewed
Nov 27, 2024
|
|
||
| namespace Microsoft.CodeAnalysis.CodeGen; | ||
|
|
||
| internal sealed class DataStringHolder : DefaultTypeDef, Cci.INamespaceTypeDefinition |
|
I stopped reviewing the PR because I think first we need to agree on the implementation strategy and have a speclet to review against. |
AlekseyTs
reviewed
Nov 27, 2024
| public override IEnumerable<DataStringHolder> GetFrozenDataStringHolders() | ||
| { | ||
| Debug.Assert(_lazyDataStringHolders is null || _dataStringHoldersFrozen == 1); | ||
| return _lazyDataStringHolders?.Values ?? SpecializedCollections.EmptyEnumerable<DataStringHolder>(); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When this opt-in feature flag is enabled by users, string literals with length over the configured threshold are emitted into data section of the PE file, hence allowing users to overcome the "error CS8103: Combined length of user strings used by the program exceeds allowed limit."
VB support is not implemented in this PR, could be added in the future if there's demand.
I've manually tried this on our unit tests (e.g., Emit2 compiled standalone resulted in significantly smaller file size due to the use of UTF8 instead of UTF16; run time was the same; combining Emit2 and Emit3 into one assembly worked without emit errors). It was also tested on aspnetcore benchmarks and that didn't reveal any significant runtime/build perf changes.
Main motivation for this feature are Razor projects which can accumulate lots of string literals over time (due to how
.razorfile compilation works) and then customers are one day hit with this error without any prior warnings (imagine they are fixing a small bug and suddenly they cannot compile anymore without refactoring their codebase into multiple projects).