Skip to content

[Chore] Remove NoInlining from ThrowHelper methods #2227

Description

@saucecontrol

Prerequisites

  • I have written a descriptive issue title
  • I have verified that I am running the latest version of ImageSharp
  • I have verified if the problem exist in both DEBUG and RELEASE mode
  • I have searched open and closed issues to ensure it has not already been reported

ImageSharp version

2.1.3

Other ImageSharp packages and versions

?

Environment (Operating system, version and so on)

All

.NET Framework version

All

Description

Opening this as a potential good-first-issue issue.

Most (all?) of the throw-only methods in this project are flagged NoInlining, which is a bit of an anti-pattern. By marking the method NoInlining, we tell the JIT not to examine the method at all when analyzing the caller. This prevents JIT from seeing that the method does nothing more than throw, which changes the way it treats the call and any branches around it.

It is better to let the JIT see into the throw-only methods so it can make good decisions around them -- it won't inline them in any case.

N.B. There is a ThrowHelper pattern that does involve NoInlining, which is described in detail in the comments here. Importantly, this pattern calls for the throwing method not to be marked NoInlining but for a method that constructs the exception (possibly doing expensive resource lookups and string building) to be. This pattern may be applicable in some places still, but generally RyuJIT will do the right thing with ThrowHelpers on its own.

Steps to Reproduce

Sharplab example

Codegen with NoInlining:

C.TestNI(System.String)
    L0000: push rsi                   ; register save because of call into the unknown
    L0001: sub rsp, 0x20
    L0005: mov rsi, rdx
    L0008: test rsi, rsi
    L000b: jne short L0012            ; throw branch assumed taken
    L000d: call 0x00007ffd14930090    ; call to ThrowHelper
    L0012: mov rcx, rsi
    L0015: add rsp, 0x20
    L0019: pop rsi                    ; register restore
    L001a: jmp 0x00007ffd0aa30228     ; tail call Console.Writeline

And without:

C.Test(System.String)
    L0000: sub rsp, 0x28
    L0004: test rdx, rdx
    L0007: je short L0015              ; throw branch assumed not-taken
    L0009: mov rcx, rdx
    L000c: add rsp, 0x28
    L0010: jmp 0x00007ffd0aa30228      ; tail call Console.Writeline
    L0015: call 0x00007ffd14930078     ; call to ThrowHelper moved to cold section
    L001a: int3                        ; inserted for debugger after a known throw

Images

No response

Activity

  1. blouflashdb commented on Sep 16, 2022

    @blouflashdb
    Contributor

    I would like to work on this.

  2. brianpopow commented on Sep 16, 2022

    @brianpopow
    Collaborator

    I would like to work on this.

    @blouflashdb: Thank you, I have assign you to this task.

  3. added a commit that references this issue on Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions