Replace LowLevelDictionary with Dictionary#29411
Conversation
danmoseley
left a comment
There was a problem hiding this comment.
I guess I need to move faster
|
Jan stated that Dictionary is more optimized but have you double checked with benchmarks? |
Which benchmarks in particular are you concerned with? |
|
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(
"å" + "Å" + "≈" + "ã" + "Ã" + "ä" + "Ä" + "„" +
"β" + "Β" + "¦" + "•" + "∩" + "ç" + "Ç" + "¸" +
"¢" + "χ" + "Χ" + "ˆ" + "♣" + "≅" + "©" + "↵" +
"∪" + "¤" + "†" + "‡" + "↓" + "⇓" + "°" + "δ" +
"Δ" + "♦" + "÷" + "é" + "É" + "ê" + "Ê" + "è" +
"È" + "∅" + " " + " " + "ε" + "Ε" + "≡" + "η" +
"Η" + "ð" + "Ð" + "ë" + "Ë" + "€" + "∃" + "ƒ" +
"∀" + "½" + "¼" + "¾" + "⁄" + "γ" + "Γ" + "≥" +
">" + "↔" + "⇔" + "♥" + "…" + "í" + "Í" + "î" +
"Î" + "¡" + "ì" + "Ì" + "ℑ" + "∞" + "∫" + "ι" +
"Ι" + "¿" + "∈" + "ï" + "Ï" + "κ" + "Κ" + "λ" +
"Λ" + "⟨" + "«" + "←" + "⇐" + "⌈" + "“" + "≤" +
"⌊" + "∗" + "◊" + "‎" + "‹" + "‘" + "<" + "¯" +
"—" + "µ" + "·" + "−" + "μ" + "Μ" + "∇" + " " +
"–" + "≠" + "∋" + "¬" + "∉" + "⊄" + "ñ" + "Ñ" +
"ν" + "Ν" + "ó" + "Ó" + "ô" + "Ô" + "œ" + "Œ" +
"ò" + "Ò" + "‾" + "ω" + "Ω" + "ο" + "Ο" + "⊕" +
"∨" + "ª" + "º" + "ø" + "Ø" + "õ" + "Õ" + "⊗" +
"ö" + "Ö" + "¶" + "∂" + "‰" + "⊥" + "φ" + "Φ" +
"π" + "Π" + "ϖ" + "±" + "£" + "′" + "″" + "∏" +
"∝" + "ψ" + "Ψ" + """ + "√" + "⟩" + "»" + "→" +
"⇒" + "⌉" + "”" + "ℜ" + "®" + "⌋" + "ρ" + "Ρ" +
"‏" + "›" + "’" + "‚" + "š" + "Š" + "⋅" + "§" +
"­" + "σ" + "Σ" + "ς" + "∼" + "♠" + "⊂" + "⊆" +
"∑" + "⊃" + "¹" + "²" + "³" + "⊇" + "ß" + "τ" +
"Τ" + "∴" + "θ" + "Θ" + "ϑ" + " " + "þ" + "Þ" +
"˜" + "×" + "™" + "ú" + "Ú" + "↑" + "⇑" + "û" +
"Û" + "ù" + "Ù" + "¨" + "ϒ" + "υ" + "Υ" + "ü" +
"Ü" + "℘" + "ξ" + "Ξ" + "ý" + "Ý" + "¥" + "ÿ" +
"Ÿ" + "ζ" + "Ζ" + "‍" + "‌", 1000));
}
}I get this before the change: and this after: 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. |
All which are affected by this change.
Thanks a lot. |
|
Reminder: If/when we migrate to BDN we probably want to add this test. |
And which are those? Seriously. Are there any that touch these code paths? I don't believe so. |
|
@dotnet/dnceng, @mmitche, legs here again appear to be stuck processing xunit results, e.g. |
|
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 🤔 |
|
Ok. Thanks. |
|
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. |
|
@stephentoub we're still having issues with the OSX machines: https://github.com/dotnet/core-eng/issues/3391 |
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. |
|
@mmitche, this isn't just macOS. |
|
@mmitche, @dotnet/dnceng, the netfx leg has been going for 9 hours 30 minutes and ends with this: Similarly the Windows x64 Debug leg has been going for over 9 hours and ends with: |
|
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 |
|
Thanks, @Chrisboh. |
* 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
Commit migrated from dotnet/corefx@d3753b7
cc: @danmosemsft, @jkotas