-
Notifications
You must be signed in to change notification settings - Fork 523
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#] Added more string read/write methods to the DirectBuffer #729 #845
Conversation
More, hopefully efficient, mechanisms to read and write strings to and from the DirectBuffer.
How is the character encoding accounted for? ASCII vs UTF-16 for example. |
In the generated code the Encoding is resovled and available: public const string ModelCharacterEncoding = "UTF-8"; And used in the generated code like this: |
@billsegall Do you have a view on this? |
@mjpt777 It looks ok but I wanted to find the time to write a couple of benchmark tests so we know it actually fixes any performance issues it is addressing. |
I'm happy to do some benchmarking and publish the results here. |
MFrpurdy That would be great thankyou |
CarBenchmark In order to compare apples to apples I modifed the CarBenchmark to encode from a string to bytes every call and decode to a string. BenchmarkDotNet=v0.12.1, OS=ubuntu 20.04 BEFORE - Modified to Encode and Decode From/To Strings Decoding example:
AFTER Encoding example: Decoding example:
|
@MFrpurdy I suspect the answer is to document the performance and leave the choice up to the user. I think the CarExample should change as you suggest as it is simpler. Could you please add the benchmarking code to the pr. I might have a play. |
…hmark which encodes and decodes to/from string; and a version wich uses the new methods to encode and decode strings
@billsegall I've added a modified CarBenchmark file that encodes to and from strings using the original methods. I've also added the same CarBenchamark test but using the new methods. |
@billsegall When you are happy with this let me know and I'll merge. |
I think the simplicity alone is worth it and people can always make performance dependent choices to fit their circumstances. |
I'd just like to comment that my company has been using a version of Rob's changes, and they were critical in terms of usability. Rob's changes cut down the amount of boilerplate code we would have had to have written to pull these strings out of complex messages - and if users want to be super careful about allocations etc., they still have the option of using the previously available methods. You can see that because the encoding is pre-computed as a static variable on assembly loading, the new methods are a bit quicker compared to a piece of code that has to work that out on the fly from the encoding string. |
@mjpt777 when does Real Logic next plan to do a release of the tool, for my information? Thanks for getting this merged. |
@rca22 Sometime within the next month. |
@mjpt777 thanks very much for doing a release of this and the other changes. Would you mind adding a new package to NuGet? This hasn't been done since 1.20.4. |
@billsegall has been doing the NuGet releases. |
I'll try and freshen a release this week |
A new release should now be availble at nuget.org |
Address #729
The suggestions that the ticket creator had require .net48+ as far as I can see - using the Encoding.GetString() methods. I kept with the current .net45 version and implmented the 'spirit' of of the request.