Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Replace LowLevelDictionary with Dictionary#29411

Merged
stephentoub merged 1 commit intodotnet:masterfrom
stephentoub:removelld
May 1, 2018
Merged

Replace LowLevelDictionary with Dictionary#29411
stephentoub merged 1 commit intodotnet:masterfrom
stephentoub:removelld

Conversation

@stephentoub
Copy link
Member

cc: @danmosemsft, @jkotas

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

I guess I need to move faster

@ViktorHofer
Copy link
Member

Jan stated that Dictionary is more optimized but have you double checked with benchmarks?

@stephentoub
Copy link
Member Author

have you double checked with benchmarks?

Which benchmarks in particular are you concerned with?

@stephentoub
Copy link
Member Author

stephentoub commented Apr 30, 2018

I couldn't find any benchmarks in the repo for WebUtility.HtmlDecode, which is one of the two places this affects, so I wrote a Benchmark.NET one:

using System;
using System.Linq;
using System.Net;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using BenchmarkDotNet.Attributes.Jobs;

namespace PerfTest
{
    [InProcess]
    public class Program
    {
        public static void Main() => BenchmarkRunner.Run<Program>();

        [Benchmark]
        public string Decode() => WebUtility.HtmlDecode(s_source);

        private static readonly string s_source = string.Concat(Enumerable.Repeat(
            "&aring;" + "&Aring;" + "&asymp;" + "&atilde;" + "&Atilde;" + "&auml;" + "&Auml;" + "&bdquo;" +
            "&beta;" + "&Beta;" + "&brvbar;" + "&bull;" + "&cap;" + "&ccedil;" + "&Ccedil;" + "&cedil;" +
            "&cent;" + "&chi;" + "&Chi;" + "&circ;" + "&clubs;" + "&cong;" + "&copy;" + "&crarr;" +
            "&cup;" + "&curren;" + "&dagger;" + "&Dagger;" + "&darr;" + "&dArr;" + "&deg;" + "&delta;" +
            "&Delta;" + "&diams;" + "&divide;" + "&eacute;" + "&Eacute;" + "&ecirc;" + "&Ecirc;" + "&egrave;" +
            "&Egrave;" + "&empty;" + "&emsp;" + "&ensp;" + "&epsilon;" + "&Epsilon;" + "&equiv;" + "&eta;" +
            "&Eta;" + "&eth;" + "&ETH;" + "&euml;" + "&Euml;" + "&euro;" + "&exist;" + "&fnof;" +
            "&forall;" + "&frac12;" + "&frac14;" + "&frac34;" + "&frasl;" + "&gamma;" + "&Gamma;" + "&ge;" +
            "&gt;" + "&harr;" + "&hArr;" + "&hearts;" + "&hellip;" + "&iacute;" + "&Iacute;" + "&icirc;" +
            "&Icirc;" + "&iexcl;" + "&igrave;" + "&Igrave;" + "&image;" + "&infin;" + "&int;" + "&iota;" +
            "&Iota;" + "&iquest;" + "&isin;" + "&iuml;" + "&Iuml;" + "&kappa;" + "&Kappa;" + "&lambda;" +
            "&Lambda;" + "&lang;" + "&laquo;" + "&larr;" + "&lArr;" + "&lceil;" + "&ldquo;" + "&le;" +
            "&lfloor;" + "&lowast;" + "&loz;" + "&lrm;" + "&lsaquo;" + "&lsquo;" + "&lt;" + "&macr;" +
            "&mdash;" + "&micro;" + "&middot;" + "&minus;" + "&mu;" + "&Mu;" + "&nabla;" + "&nbsp;" +
            "&ndash;" + "&ne;" + "&ni;" + "&not;" + "&notin;" + "&nsub;" + "&ntilde;" + "&Ntilde;" +
            "&nu;" + "&Nu;" + "&oacute;" + "&Oacute;" + "&ocirc;" + "&Ocirc;" + "&oelig;" + "&OElig;" +
            "&ograve;" + "&Ograve;" + "&oline;" + "&omega;" + "&Omega;" + "&omicron;" + "&Omicron;" + "&oplus;" +
            "&or;" + "&ordf;" + "&ordm;" + "&oslash;" + "&Oslash;" + "&otilde;" + "&Otilde;" + "&otimes;" +
            "&ouml;" + "&Ouml;" + "&para;" + "&part;" + "&permil;" + "&perp;" + "&phi;" + "&Phi;" +
            "&pi;" + "&Pi;" + "&piv;" + "&plusmn;" + "&pound;" + "&prime;" + "&Prime;" + "&prod;" +
            "&prop;" + "&psi;" + "&Psi;" + "&quot;" + "&radic;" + "&rang;" + "&raquo;" + "&rarr;" +
            "&rArr;" + "&rceil;" + "&rdquo;" + "&real;" + "&reg;" + "&rfloor;" + "&rho;" + "&Rho;" +
            "&rlm;" + "&rsaquo;" + "&rsquo;" + "&sbquo;" + "&scaron;" + "&Scaron;" + "&sdot;" + "&sect;" +
            "&shy;" + "&sigma;" + "&Sigma;" + "&sigmaf;" + "&sim;" + "&spades;" + "&sub;" + "&sube;" +
            "&sum;" + "&sup;" + "&sup1;" + "&sup2;" + "&sup3;" + "&supe;" + "&szlig;" + "&tau;" +
            "&Tau;" + "&there4;" + "&theta;" + "&Theta;" + "&thetasym;" + "&thinsp;" + "&thorn;" + "&THORN;" +
            "&tilde;" + "&times;" + "&trade;" + "&uacute;" + "&Uacute;" + "&uarr;" + "&uArr;" + "&ucirc;" +
            "&Ucirc;" + "&ugrave;" + "&Ugrave;" + "&uml;" + "&upsih;" + "&upsilon;" + "&Upsilon;" + "&uuml;" +
            "&Uuml;" + "&weierp;" + "&xi;" + "&Xi;" + "&yacute;" + "&Yacute;" + "&yen;" + "&yuml;" +
            "&Yuml;" + "&zeta;" + "&Zeta;" + "&zwj;" + "&zwnj;", 1000));
    }
}

I get this before the change:

 Method |     Mean |     Error |    StdDev |
------- |---------:|----------:|----------:|
 Decode | 19.11 ms | 0.3691 ms | 0.4250 ms |

and this after:

 Method |     Mean |     Error |    StdDev |
------- |---------:|----------:|----------:|
 Decode | 18.29 ms | 0.0722 ms | 0.0564 ms |

so as far as I can tell, there's no regression here. This change isn't about making it go faster, it's about getting rid of unnecessary code.

@ViktorHofer
Copy link
Member

Which benchmarks in particular are you concerned with?

All which are affected by this change.

so as far as I can tell, there's no regression here

Thanks a lot.

@ViktorHofer
Copy link
Member

Reminder: If/when we migrate to BDN we probably want to add this test.

@stephentoub
Copy link
Member Author

All which are affected by this change.

And which are those? Seriously. Are there any that touch these code paths? I don't believe so.

@stephentoub
Copy link
Member Author

@dotnet/dnceng, @mmitche, legs here again appear to be stuck processing xunit results, e.g.

[Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Response Code: HTTP/1.1 200 OK
[Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Response: 
[Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] [{"Key":{"job.name":"fe2ca341-14f6-43ce-9119-8dce032bd1c2"},"Data":{"Analysis":[{"Name":"xunit","Status":{"pass":250713,"passonretry":0,"fail":0,"skip":1336}}],"WorkItemStatus":{"pass":205,"none":1}}}]
[Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Success code from [100‥399]
[Pipeline] [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] echo
[Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Info: Processing XUnit Results: 1 remaining
[Pipeline] [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] }
[Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Will try again after 5 min 0 sec

@ViktorHofer
Copy link
Member

I don't think there are any. When I said "have you double checked with benchmarks?" I meant if you either run existing ones or if there weren't any if you wrote one to make sure there's no regression. Sorry for the confusion 🤔

@stephentoub
Copy link
Member Author

Ok. Thanks.

@morganbr
Copy link

I believe some of the CoreLib cases also affect the size of .NET Native binaries, so it would be good to confirm there's no significant expansion if you change those.

@mmitche
Copy link
Member

mmitche commented Apr 30, 2018

@stephentoub we're still having issues with the OSX machines: https://github.com/dotnet/core-eng/issues/3391
/cc @Chrisboh

@jkotas
Copy link
Member

jkotas commented Apr 30, 2018

I believe some of the CoreLib cases also affect the size of .NET Native binaries, so it would be good to confirm there's no significant expansion if you change those.

I have tried that earlier and had to roll it back: dotnet/corert#5234 . It did not even work because of it introduced cycles somewhere.

@stephentoub
Copy link
Member Author

@mmitche, this isn't just macOS.

@stephentoub
Copy link
Member Author

stephentoub commented May 1, 2018

@mmitche, @dotnet/dnceng, the netfx leg has been going for 9 hours 30 minutes and ends with this:
https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netfx+CGroup_Release+AGroup_x86+TestOuter_false_prtest/11839/

20:59:17 [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Response Code: HTTP/1.1 200 OK
20:59:17 [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Response: 
20:59:17 [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] [{"Key":{"job.name":"fe2ca341-14f6-43ce-9119-8dce032bd1c2"},"Data":{"Analysis":[{"Name":"xunit","Status":{"pass":250713,"passonretry":0,"fail":0,"skip":1336}}],"WorkItemStatus":{"pass":205,"none":1}}}]
20:59:17 [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Success code from [100‥399]
[Pipeline] [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] echo
20:59:17 [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Info: Processing XUnit Results: 1 remaining
[Pipeline] [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] }
20:59:17 [Windows.10.Amd64.ClientRS2.Open - x86 - Release (fe2ca341-14f6-43ce-9119-8dce032bd1c2)] Will try again after 5 min 0 sec

Similarly the Windows x64 Debug leg has been going for over 9 hours and ends with:
https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/12019/

[Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] Response Code: HTTP/1.1 200 OK
[Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] Response: 
[Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] [{"Key":{"job.name":"ac25e449-3190-41e6-9a04-a29ca7caaf8f"},"Data":{"Analysis":[{"Name":"xunit","Status":{"pass":282735,"passonretry":0,"fail":0,"skip":3254}}],"WorkItemStatus":{"pass":252,"none":1}}}]
[Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] Success code from [100‥399]
[Pipeline] [Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] echo
[Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] Info: Processing XUnit Results: 1 remaining
[Pipeline] [Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] }
[Windows.10.Nano.Amd64.Open - x64 - Debug (ac25e449-3190-41e6-9a04-a29ca7caaf8f)] Will try again after 5 min 0 sec

@Chrisboh
Copy link
Member

Chrisboh commented May 1, 2018

Sorry for the slow response @stephentoub we are a bit shorthanded this week. @jcagme is looking into this now https://github.com/dotnet/core-eng/issues/3151

@stephentoub
Copy link
Member Author

Thanks, @Chrisboh.

@stephentoub stephentoub merged commit d3753b7 into dotnet:master May 1, 2018
@stephentoub stephentoub deleted the removelld branch May 1, 2018 17:16
@karelz karelz added this to the 2.2.0 milestone May 1, 2018
morganbr pushed a commit to morganbr/corefx that referenced this pull request Jul 7, 2018
zacharycmontoya pushed a commit that referenced this pull request Aug 14, 2018
* Manually update the version of Microsoft.TargetingPack.Private.NETNative

* Set Microsoft.TargetPack.Private.NETNative version to track live TFS version

* Move Hashtable & friends to shared parition

Include serialization info table for Hashtable

Remove hashtable & hashprovider src link

* Compile hashtable & friends only on uapaot

* Fix build breaks

* Remove Opcodes due to Opcodes moved to S.P.Corelib

* Move Hashtable & friends to shared parition (dotnet/coreclr#17316) (#29307)

* Move Hashtable & friends to shared parition

* Move HashHelper serialization logic into its own file

* Remove unchecked keyword in Hashtable

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

* Moving ConcurrentQueue to shared folder (dotnet/coreclr#17956)

* Moving ConcurrentQueue to shared folder

Fixes: #17751

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

* Moving ConcurrentQueue to shared (dotnet/coreclr#18024)

Making IProducerConsumerCollectionDebugView apis public

Fixes: #17751

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

* Modify corefx to use ConcurrentQueue from shared

* Delete dead code after Hashtable.cs move (#29432)

* Replace LowLevelDictionary with Dictionary (#29411)

* Avoid substring allocations in WebUtility.HtmlDecode (#29402)

* Avoid substring allocations in WebUtility.HtmlDecode

* Update changes to HtmlDecode based on feedback

- Use regular Dictionary instead of LowLevelDictionary
- Use AsSpan overload instead of AsSpan() and Slice
- Avoid shift by variable amount
- Use helper method to generate keys to make the code easier to maintain

* Assert that entity length is <= 8 in ToUInt64Key

- Add assert to ToUInt64Key
- Replace default with 0

* Remove CoreSetup, External, and Sni dependencies

* Update the CoreFx dependency to release/2.1 so that Microsoft.NETCore.Platforms is defined correctly. Manually invoke the dependency update

* Removing Fedora 26 and adding 28 as appropriate.  Fix a couple README.md typos.

* Remove Ubuntu 17.10 from CoreFX official runs
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants