Optimize unboxing in System.Data.Common.ObjectStorage:Set

[xtqqczze]

Inline value.GetType() instead of using a local, to enable a JIT optimization.

Diffs: https://www.diffchecker.com/QgpKJ9UD/

Top method improvements (bytes):
        -456 (-32.50 % of base) : System.Data.Common.dasm - System.Data.Common.ObjectStorage:Set(int,System.Object):this (FullOpts)

[ghost]

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

Issue Details

---

Emitting isinst + unbox.any enables a JIT optimization.

Author:	xtqqczze
Assignees:	-
Labels:	area-System.Data, community-contribution
Milestone:	-

[roji]

nit: can use a switch expression

[huoyaoyuan]

This may hit the performance issue logged in #47920. I actually have a branch to improve this, but it's not expandable-proof so I haven't push it.

It's also interesting to see if a following unbox.any changes the codegen of isinst.

[xtqqczze]

In the case of a switch expression, the JIT is not able to eliminate the calls to CORINFO_HELP_UNBOX.

[huoyaoyuan]

I'm surprised that CORINFO_HELP_UNBOX is ever evolved after #87241.

[xtqqczze]

I'm surprised that CORINFO_HELP_UNBOX is ever evolved after #87241.

The isinst + unbox.any optimization appears in .NET 7.

[EgorBo]

there should be no perf difference between valType == typeof(..) and val is PrimitiveType I believe, at least for JIT.

[xtqqczze]

there should be no perf difference between valType == typeof(..) and val is PrimitiveType I believe, at least for JIT.

This is part of the current codegen, from the build MihuBot/runtime-utils#177, showing a call to CORINFO_HELP_UNBOX that could be eliminated.

G_M32004_IG28:
       mov      rsi, 0xD1FFAB1E      ; 'System.UInt64'

       cmp      rax, rsi
       jne      SHORT G_M32004_IG31
       mov      r15, gword ptr [r15+0x40]
       mov      rsi, 0xD1FFAB1E      ; System.UInt64
       cmp      qword ptr [rbx], rsi
       je       SHORT G_M32004_IG30
						;; size=34 bbWeight=0.50 PerfScore 3.88
G_M32004_IG29:
       mov      rsi, rbx
       mov      rdi, 0xD1FFAB1E      ; System.UInt64
       mov      rax, 0xD1FFAB1E      ; code for CORINFO_HELP_UNBOX
       call     [rax]CORINFO_HELP_UNBOX
						;; size=25 bbWeight=0.25 PerfScore 0.94
G_M32004_IG30:
       mov      rdi, qword ptr [rbx+0x08]
       mov      rax, 0xD1FFAB1E      ; code for System.BitConverter:GetBytes(ulong):ubyte[]
       call     [rax]System.BitConverter:GetBytes(ulong):ubyte[]
       mov      rdx, rax
       movsxd   rsi, r14d
       mov      rdi, r15
       call     CORINFO_HELP_ARRADDR_ST
       jmp      G_M32004_IG37
						;; size=35 bbWeight=0.50 PerfScore 4.50

The issue may be that the JIT is unable to prove value is not null.

[xtqqczze]

@EgorBo This change is also a little more efficient in terms of IL.

  isinst [System.Runtime]System.Single

vs:

  callvirt instance class [System.Runtime]System.Type [System.Runtime]System.Object::GetType()
  ldtoken [System.Runtime]System.Single
  call class [System.Runtime]System.Type [System.Runtime]System.Type::GetTypeFromHandle(valuetype [System.Runtime]System.RuntimeTypeHandle)
  call bool [System.Runtime]System.Type::op_Equality(class [System.Runtime]System.Type, class [System.Runtime]System.Type)

[xtqqczze]

@MihuBot

[xtqqczze]

@MihuBot

[huoyaoyuan]

I think we would be able to use switch when #47920 is fully resolved.

[xtqqczze]

@MihuBot

[MihaZupan]

What is the aim behind replacing else blocks with many duplicated returns?

[xtqqczze]

To use a single throw at the end of the method whilst keeping the control flow simple. The JIT will create a single exit.

If this is less readable I can revert to else blocks.

[xtqqczze]

@MihaZupan What is your view on this?

[xtqqczze]

there should be no perf difference between valType == typeof(..) and val is PrimitiveType I believe, at least for JIT.

@EgorBo This is correct, as long as valType isn't stored to a local.

I've refactored to use value.GetType() inline.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.