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

[C#] Replace regex that validates descriptor names #12174

Closed
wants to merge 3 commits into from

Conversation

JamesNK
Copy link
Contributor

@JamesNK JamesNK commented Mar 8, 2023

This PR replaces the descriptor name validation regex with a validation method. This change allows the System.Text.RegularExpressions engine to be trimmed away in published apps that do standard protobuf serialization.

There are some other usages of Regex in Google.Protobuf, but they in JsonParser. They are only included in a published app if JsonParser is used.

Another benefit is a slightly faster app startup time. The removed regex was compiled, which has a high-ish fixed cost.

cc @jskeet @jtattermusch

@JamesNK JamesNK requested a review from a team as a code owner March 8, 2023 12:17
@JamesNK JamesNK requested review from jskeet and removed request for a team March 8, 2023 12:17
Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Definitely happy with the overall aim - a few nits.

@JamesNK
Copy link
Contributor Author

JamesNK commented Mar 8, 2023

All feedback applied.

@jskeet jskeet added c# 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Mar 8, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Mar 8, 2023
@JamesNK
Copy link
Contributor Author

JamesNK commented Mar 21, 2023

Is this waiting on more reviews? I just want to make sure it doesn't get left behind.

For context, this change saves 1 MB from .NET Native gRPC client and server apps. I'd love to show off the smallest gRPC apps possible in demos 😄

@jskeet
Copy link
Contributor

jskeet commented Mar 21, 2023

Thanks for the prod - I'll chase it up.

@jskeet jskeet requested a review from deannagarcia March 21, 2023 07:46
@jskeet
Copy link
Contributor

jskeet commented Mar 21, 2023

@deannagarcia: I think your approval (or at least, someone actually on the Protobuf team) is required before this can proceed.

@JamesNK
Copy link
Contributor Author

JamesNK commented May 10, 2023

This missed 3.23.0.

Could it be merged now to make it into the next protobuf release?

@jskeet
Copy link
Contributor

jskeet commented May 10, 2023

@JamesNK: Aargh, sorry that this fell through the cracks.
@deannagarcia: Please could you merge?

@deannagarcia
Copy link
Member

Sorry this fell through, working to get it in now.

@deannagarcia deannagarcia added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 15, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 15, 2023
@deannagarcia deannagarcia force-pushed the jamesnk/remove-regex branch from 832909d to 9d065a3 Compare May 15, 2023 17:00
@deannagarcia deannagarcia added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 15, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 15, 2023
mkruskal-google pushed a commit that referenced this pull request May 15, 2023
This PR replaces the descriptor name validation regex with a validation method. This change allows the `System.Text.RegularExpressions` engine to be trimmed away in published apps that do standard protobuf serialization.

There are some other usages of `Regex` in Google.Protobuf, but they in `JsonParser`. They are only included in a published app if `JsonParser` is used.

Another benefit is a slightly faster app startup time. The removed regex was compiled, which has a high-ish fixed cost.

cc @jskeet @jtattermusch

Closes #12174

COPYBARA_INTEGRATE_REVIEW=#12174 from JamesNK:jamesnk/remove-regex 9d065a3
PiperOrigin-RevId: 532210203
@jskeet
Copy link
Contributor

jskeet commented May 18, 2023

@JamesNK: This went out in 3.23.1 :)

@JamesNK
Copy link
Contributor Author

JamesNK commented May 18, 2023

Nice. Thanks!

@robertodalmonte
Copy link

Hi there,
This morning I updated my WPF project from:

to

This is the only change I made.
I published my project using my usual settings:


Release
Any CPU
D:\Github\OneClick\OneClick.Dental\bin\publish
FileSystem
<_TargetId>Folder</_TargetId>
net7.0-windows
win-x64
false
true
true


If I try to launch my WPF client it stops as soon as trying to connect to my gRPC server.
It does not happen if I do the same from Visual Studio in debug or if I launch the exe from the release folder after building.
I rolled back to
and I can publish my WPF project successfully and I'm able to connect as usual to my gRPC Server app.
Is it possible that the last changes might have affected publishing?
Should I look somewhere else?
Best regards
Roberto

@robertodalmonte
Copy link

from 3.23 to 3.23.1

@jskeet
Copy link
Contributor

jskeet commented May 18, 2023

@robertodalmonte: Please file a new issue in https://github.com/grpc/grpc-dotnet with more details - it's not appropriate to use a closed PR to report an issue, even if the issue was caused by the change.

I'd also strongly recommend that in the new issue, you give more detail than "it stops". I expect there's an exception being thrown somewhere - please look for that and give full details of it in the issue.

@robertodalmonte
Copy link

Thanks for your suggestion, I will follow it.
I'm sorry for the inappropriate use, it wasn't meant to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants