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.

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

Change Configuration.Json to use a regular Dictionary. #50611

Merged
merged 1 commit into from
Apr 5, 2021

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Apr 1, 2021

The other Configuration providers use a regular Dictionary.

This allows for SortedDictionary to be trimmed in a default Blazor WASM app, saving roughly 4 KB .br compressed.

The other Configuration providers use a regular Dictionary.

This allows for SortedDictionary to be trimmed in a default Blazor WASM app, saving roughly 4 KB .br compresse.
@ghost
Copy link

ghost commented Apr 1, 2021

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

The other Configuration providers use a regular Dictionary.

This allows for SortedDictionary to be trimmed in a default Blazor WASM app, saving roughly 4 KB .br compressed.

Author: eerhardt
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@eerhardt eerhardt added the size-reduction Issues impacting final app size primary for size sensitive workloads label Apr 1, 2021
@ghost
Copy link

ghost commented Apr 1, 2021

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @tannergooding, @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

The other Configuration providers use a regular Dictionary.

This allows for SortedDictionary to be trimmed in a default Blazor WASM app, saving roughly 4 KB .br compressed.

Author: eerhardt
Assignees: -
Labels:

area-Extensions-Configuration, size-reduction

Milestone: -

@stephentoub
Copy link
Member

If the order of the data returned to the consumer isn't important, LGTM. But I wonder why this was using a SortedDictionary in the first place.

@davidfowl
Copy link
Member

I'm wondering if it was trying to preserve ordering from the original file. The way things would be overwritten is preserved?

@eerhardt
Copy link
Member Author

eerhardt commented Apr 2, 2021

If the order of the data returned to the consumer isn't important, LGTM. But I wonder why this was using a SortedDictionary in the first place.

I'm really not sure. I looked at the original PR aspnet/Configuration#186, and there were no comments why it was using sorted. There isn't a test for it to be sorted.

Also, the other file configuration providers use a regular Dictionary:

var configuration = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
if (root == null)
{
return configuration;
}
var rootPrefix = new Prefix();
// The root element only contributes to the prefix via its Name attribute
if (!string.IsNullOrEmpty(root.Name))
{
rootPrefix.Push(root.Name);
}
ProcessElementAttributes(rootPrefix, root);
ProcessElementContent(rootPrefix, root);
ProcessElementChildren(rootPrefix, root);
return configuration;

public static IDictionary<string, string> Read(Stream stream)
{
var data = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
using (var reader = new StreamReader(stream))

I'm not sure how to find the answer. Is there someone from the ASP.NET team that is an expert on configuration?

@safern
Copy link
Member

safern commented Apr 2, 2021

Maybe @HaoK can help here.

@HaoK
Copy link
Member

HaoK commented Apr 2, 2021

I don't have any context here either as this code was before my time on config as well. The order of the keys for any particular configuration provider shouldn't matter as its just the existence of a key that determines whether its a hit for config lookup purposes. I guess the relevant method that might be affected is GetChildKeys which also seems to do some sorting...

results.Sort(ConfigurationKeyComparer.Comparison);

@davidfowl
Copy link
Member

I spoke to the original author and it seems like this may not have been necessary. Carry on.

@HaoK
Copy link
Member

HaoK commented Apr 2, 2021

I was about to say, lets ask Lou if he remembers :)

@davidfowl
Copy link
Member

Wait did Lou write this, I asked victor 🤣

@HaoK
Copy link
Member

HaoK commented Apr 2, 2021

Nope, you are right, it was Victor, i found the commit: aspnet/Configuration@0517eee

@victorhurdugaci
Copy link

victorhurdugaci commented Apr 2, 2021

👋 I have no idea why I wanted the keys to be sorted. I told David that my best guess is that I was trying to preserve the order of elements in arrays but, if that was my intention, the implementation is buggy past 10 elements... So 🤷 sorry...

@davidfowl
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@safern safern merged commit 433ce26 into dotnet:main Apr 5, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Apr 6, 2021
…shim_mono

# By Aaron Robinson (10) and others
# Via GitHub
* upstream/main: (108 commits)
  [mbr] Add Apple sample (dotnet#50740)
  make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763)
  Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622)
  [mono] More domain cleanups (dotnet#50479)
  Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754)
  Disable EventSource generator in design-time builds (dotnet#50741)
  Fix X509 test failures on Android (dotnet#50301)
  Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703)
  Enforce 64KB event payload size limit on EventPipe  (dotnet#50600)
  Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906)
  [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458)
  improve connection scavenge logic by doing zero-byte read (dotnet#50545)
  Resolve call mdtokens when making tier 1 inline observations (dotnet#50675)
  Annotate APIs in System.Private.Xml (dotnet#49682)
  Support compiling against OpenSSL 3 headers
  Change Configuration.Json to use a regular Dictionary. (dotnet#50611)
  Remove unused BigNumFromBinary P/Invoke (dotnet#50670)
  Make Ninja the default CMake generator on Windows for the repo (dotnet#49715)
  [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637)
  [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547)
  ...

# Conflicts:
#	src/mono/dlls/mscordbi/CMakeLists.txt
@eerhardt eerhardt deleted the RemoveSortedDictionary branch April 6, 2021 16:09
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants