Fix invalid Box<TKey> and number dereference in dict. implementations... #23
Conversation
- By the time we reach _comparer(key.Value, entryKey.Value) where entryKey is `ref Box<TKey>`, the reference might have been changed, leading to catching null reference exception or obvserving wrong value. Solve this by de-referencing `key` exactly once within method body. Also solve this for number primitive implementations. - Speed up tests by using Parallel.For over Parallel.ForEach(enumerable) - Slow down tests back by increasing concurrent iterations count for reliability, reduce code duplication - Fix CLI builds by removing IsTrimmable, dotnet sdk seems to have gotten unhappy about it when targeting NS2.0 or 2.1 - Do not cache typeof(TValue).IsValueType because it is a JIT constant - Remove TC off from builds, it has never really been an issue with default configuration and JIT can now do either OSR or classic TC when there is no loop, enable DynamicPGO too
|
With all previous comments resolved, is there anything else I need to address? |
|
I am getting somewhat weird results with dynamic PGO enabled. without PGO I see roughly with PGO I see I.E. It looks like we get a huge improvement in the throughput from the PGO, but it takes tens of seconds and millions of hashtable Get operations before the benefits kick in. Before that happens the perf is pretty poor. I am inclined to keep PGO enabled in the benchmark project, since it results in a better perf and thus should be the desired mode for which we optimize. Besides it is just the default, which is easy to change and is often changed during perf investigations anyways, but I wonder if what I see is a normal behavior. CC: @EgorBo - is it normal for PGO effects to be so delayed? |
@neon-sunset I do not see anything else that needs changing. |
Can you try again with |
does not seem to have any effect |
- Don't cache typeof(T).IsValueType in a duplicate FromObjectValue either - Simplify throw helper code
Ouch, but I see it's net7, right? We've landed many improvements for startup in net8 - can you try with at least net8.0 preview 5 (or later) ? |
Right. 8.0 does not have the issue! And it seems the overall performance is a bit better too. |
By the time we reach
_keyComparer.Equals(key.Value, entryKey.Value)inTryClaimSlotForPut/CopywhereentryKeyisref Box<TKey>, the reference might have been changed, leading to catching null reference exception or observing wrong value. Solve this by passing initially dereferencedentryKeyValueinstead, as was probably intended in the first place. Fix this for number primitive implementations too.Additionally:
Fixes neon-sunset/fast-cache#52