Skip to content

Use ThrowHelper to throw exceptions (part 1)#4872

Merged
vonzshik merged 5 commits into
mainfrom
2237-use-throwhelper-part-1
Jan 13, 2023
Merged

Use ThrowHelper to throw exceptions (part 1)#4872
vonzshik merged 5 commits into
mainfrom
2237-use-throwhelper-part-1

Conversation

@vonzshik

@vonzshik vonzshik commented Jan 12, 2023

Copy link
Copy Markdown
Contributor

Contributes to #2237

Draft until I check the code jit compiles.

@gfoidl

gfoidl commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

Just as info: with .NET 8 alot of throw-helpers come inbox (ArgumentOutOfRange, ObjectDisposed, etc.).
So I think it's good to have same signatures in the ThrowHelper here, so that an update later is easy for the call-sites.

@Brar

Brar commented Jan 13, 2023

Copy link
Copy Markdown
Member

Maybe we should already add the net8.0 TFM and make this a shim.

@vonzshik

Copy link
Copy Markdown
Contributor Author

I'm marking the PR as ready for review. There are going to be multiple parts to make it easire to review.

Adding .NET 8 TFM and referencing inbox throw-helpers should probably be done in a separate pr.

One interesting thing I've encountered while working on this PR is that jit generates a lot of code for string interpolation even if the argument is a constant (though nameof works fine).

@vonzshik vonzshik marked this pull request as ready for review January 13, 2023 12:44
@vonzshik vonzshik requested a review from roji as a code owner January 13, 2023 12:44
@roji

roji commented Jan 13, 2023

Copy link
Copy Markdown
Member

One interesting thing I've encountered while working on this PR is that jit generates a lot of code for string interpolation even if the argument is a constant (though nameof works fine).

Interesting... The nameof case shows that it does the right thing for at least some constants, but for fails with ushort.MaxValue... Maybe because it's a non-string and so requires a conversion, and Roslyn doesn't do that?

I'd flag this on https://github.com/dotnet/roslyn (assuming there's not already an issue)

Aside from that there are some test failures...

@vonzshik

Copy link
Copy Markdown
Contributor Author

Aside from that there are some test failures...

Some issue with downloading PG, gonna restart...

@gfoidl

gfoidl commented Jan 13, 2023

Copy link
Copy Markdown
Contributor

it's a non-string and so requires a conversion, and Roslyn doesn't do that?

For codegen look at what Roslyn generated.
Right now $"ABC{ushort.MaxValue}" is an interpolated string, and not evaluated at compile-time. Thus lots of code.
C# / Roslyn wise I think dotnet/csharplang#2379 is the issue tracking this (there are soo many over there...).

For strings in interpolation holes C# 10 has the feature of "constant interpolated strings".
So all other types are normal interpolated strings, with runtime evaluation.

@vonzshik

Copy link
Copy Markdown
Contributor Author

For codegen look at what Roslyn generated.
Right now $"ABC{ushort.MaxValue}" is an interpolated string, and not evaluated at compile-time. Thus lots of code.
C# / Roslyn wise I think dotnet/csharplang#2379 is the issue tracking this (there are soo many over there...).

For strings in interpolation holes C# 10 has the feature of "constant interpolated strings".
So all other types are normal interpolated strings, with runtime evaluation.

It seems like integers (and I guess that includes uint) aren't a constant interpolated string because of a formatting differences across cultures?

dotnet/csharplang#2951 (comment)

@roji

roji commented Jan 13, 2023

Copy link
Copy Markdown
Member

It seems like integers (and I guess that includes uint) aren't a constant interpolated string because of a formatting differences across cultures?

Ah yeah, that makes sense. Though if memory serves, there's a way to specify invariant culture inside the interpolation, and that could be done at compile-time.

@roji roji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am fine with this, though I wonder if it's worth systematically doing this for the entire codebase... In other words, there are methods which are never going to get inlined anyway (because they're big), and so doing this there doesn't seem to have much value; in some places it also makes code readability worse (because we can't throw from expression context).

Comment thread src/Npgsql/BackendMessages/RowDescriptionMessage.cs
@vonzshik

Copy link
Copy Markdown
Contributor Author

Am fine with this, though I wonder if it's worth systematically doing this for the entire codebase... In other words, there are methods which are never going to get inlined anyway (because they're big), and so doing this there doesn't seem to have much value; in some places it also makes code readability worse (because we can't throw from expression context).

Yeah, I'm definitely not going to remove them from everywhere any time soon)
Probably just concentrate on the "open_connection->execute_query->close_connection" path for now (I think the good idea will be to get rid of string interpolation).
The point here isn't only about inlining, but to also make the generated code smaller (this can allow jit to use less registers and operations, string interpolation is prime example of that).

@vonzshik vonzshik merged commit 6a02236 into main Jan 13, 2023
@vonzshik vonzshik deleted the 2237-use-throwhelper-part-1 branch January 13, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants