Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ThrowHelper instead of inline exception throwing #2237

Open
YohDeadfall opened this issue Nov 21, 2018 · 6 comments
Open

Use ThrowHelper instead of inline exception throwing #2237

YohDeadfall opened this issue Nov 21, 2018 · 6 comments

Comments

@YohDeadfall
Copy link
Contributor

YohDeadfall commented Nov 21, 2018

The idea of the proposed change is to:

  1. Make code generated methods smaller because the JIT emits jump to throw methods and doesn't inline them. The description of the trick can be found here.
  2. Prevent duplication of exception messages.
@roji
Copy link
Member

roji commented Nov 21, 2018

Just to clarify, are you expecting this issue to cover all exceptions thrown in npgsql, everywhere?

@YohDeadfall
Copy link
Contributor Author

As the starting point it should be done for hot paths (connections, commands, handlers). After that we could consider about other parts of the project.

@roji
Copy link
Member

roji commented May 6, 2019

Thanks for reminding me about this.

If we really want to prevent duplication of exception messages, it seems like that can/should be done with a standard "Strings" (i.e. resx). At least in theory it would also allow localization down the road (although I'm not too enthusiastic about that).

For the perf side:

  1. In Added property access checks in composite handler #2451 you didn't separate between two methods, i.e. a void-returning inlinable and an exception-returning non-inlinable, as in https://github.com/dotnet/corefx/blob/56dfb8834fa50f3bc61ea9b4bfdc9dcc759b6ec9/src/System.Memory/src/System/ThrowHelper.cs#L11-L22. Is that intentional?
  2. Note also that https://github.com/dotnet/corefx/blob/56dfb8834fa50f3bc61ea9b4bfdc9dcc759b6ec9/src/System.Memory/src/System/ThrowHelper.cs#L11-L22 is about "a compromise between older JITs and newer JITs", and so I'm not sure this pattern would make sense today in Npgsql.

Maybe set up a quick benchmark to see if it still makes sense?

@roji
Copy link
Member

roji commented Oct 25, 2020

@YohDeadfall do you still intend to do this for 5.0? If not, can we please move this to the backlog (or 6.0)?

@roji
Copy link
Member

roji commented Nov 1, 2020

@YohDeadfall I'm going to move this out to vNext. If you get around to doing this for 5.0 we can always move it back.

@roji roji modified the milestones: 5.0.0, vNext Nov 1, 2020
@YohDeadfall
Copy link
Contributor Author

Am fine with it.

@YohDeadfall YohDeadfall removed their assignment Feb 17, 2021
@vonzshik vonzshik modified the milestones: 6.0.0, 7.0.0 Oct 3, 2021
@roji roji modified the milestones: 7.0.0, Backlog Oct 5, 2021
vonzshik added a commit that referenced this issue Jan 13, 2023
vonzshik added a commit that referenced this issue Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants