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

initial prove of concept less string allocations #1826

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JanEggers
Copy link

@JanEggers JanEggers commented Jul 7, 2021

fixed #1825

I used the benchmark project to generate a file with 1000000 records and then actually run the benchmark to read them with

using (var parser = new CsvReader(reader, config))
{
    foreach (var item in parser.GetRecords<Program.Columns50>())
    {
    
    }
}
Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated branch
Parse 7.599 s 0.1678 s 0.4759 s 1176000.0000 - - 4.58 GB master
Parse 4.746 s 0.0875 s 0.1622 s 338000.0000 - - 1.32 GB pr

+ 40% speed
-100% string allocations when reading records
-100% type[] allocation with default ctor

if the general design is accepted i will implement more type converters and come up with a way to check wether a row should be skipped without allocating

@JanEggers JanEggers changed the title initial prove of concept initial prove of concept less string allocations Jul 7, 2021
@JoshClose
Copy link
Owner

Can you also benchmark with config option CacheFields = true for each of those and add them to your results?

@JanEggers
Copy link
Author

sure

@JanEggers
Copy link
Author

i modified the testset to generate 100_000 rows with random int values as just having 50 values repeating over and over is not realistic

here are the numbers with caching enabled:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated branch
Parse 4.165 s 0.0830 s 0.1558 s 72000.0000 29000.0000 7000.0000 708.96 MB master
Parse 1.138 s 0.0226 s 0.0532 s 33000.0000 - - 135.36 MB pr

@JoshClose
Copy link
Owner

Dang. I was planning on doing this at some point so it's great that you could do it. This will be a breaking change so I may deploy with some other changes. We'll see.

@JanEggers
Copy link
Author

I would really love to also support netstandard and net4.5 properly but i didnt find a parser. the spanbased parser overloads are only present in netcoreapp

@JoshClose
Copy link
Owner

You might be able to use this. https://www.nuget.org/packages/System.Memory/

@JoshClose
Copy link
Owner

There is a lot more work that needs to go into this. All the type converters would need to be switched to use Span. I would think the parser could just return a ReadOnlySpan instead of strings. The consumer could just to .ToString() if they want.

@JoshClose
Copy link
Owner

CacheFields does nothing when using Span. The field cache stores created instances of strings and returns them instead of calling new. I'm curious the speed difference when getting records normally with CacheFields on and then when using Span. Will caching the strings be faster than not allocating the strings to begin with.

@JoshClose
Copy link
Owner

Seems to be a lot slower when using the field cache. If the data is duplicated, the field cache should help, otherwise it's a lot more processing.

@JanEggers
Copy link
Author

JanEggers commented Jul 13, 2021

You might be able to use this. https://www.nuget.org/packages/System.Memory/

I will try it. but the utf8parser assumes utf8 encoding so we would need to convert span of char to span of byte which needs to be allocated / arraypooled and also takes some time.

There is a lot more work that needs to go into this.

I agree I was just proposing the design to get feedback.

I would think the parser could just return a ReadOnlySpan instead of strings.

Sure thats possible but breaking. with my approach all old converters (also the ones of library consumers) still work and the span based converter is an opt in. Im also fine with just converting all string returning methods to span as it will not pollute the api with a bunch more overloads

Will caching the strings be faster than not allocating the strings to begin with.

no caching strings is not faster see my second benchmark post.
I didnt use the field cache as it makes no sense to cache a span as its just a pointer into existing data (very low cost) and just calculating the hash and sequenceequals in the cache lookup is more expensive than just gettting the span. also using the fieldcache with many different values you get gen2 allocations wich is not desirable

Another idea would be to cache the converted object instead of the raw value that way you could eliminate the converter call.
But that may need some benchmarking as well as im ot sure whether int.parse and similar are actually slower than the cache lookup

@JoshClose
Copy link
Owner

Another idea would be to cache the converted object instead of the raw value that way you could eliminate the converter call.

That could be an issue if people are expecting a new object each time. If they have a list of objects and edit one, they could be editing multiple without knowing.

Also, that is exactly what EnumerateRecords<T>(T record) does. You supply the object and it gets filled in.

Maybe you were talking about the member values and not the object itself. That's an interesting idea. I think that would work fine with Span also.

@JanEggers
Copy link
Author

Maybe you were talking about the member values and not the object itself.

yup that is what i meant to say

@JoshClose
Copy link
Owner

I will try it. but the utf8parser assumes utf8 encoding so we would need to convert span of char to span of byte which needs to be allocated / arraypooled and also takes some time.

I don't understand. What is utf8parser?

@JanEggers
Copy link
Author

the parser in that nuget

@JoshClose
Copy link
Owner

Oh... I just meant to be able to use Span on the older frameworks.

@JanEggers
Copy link
Author

thats working fine with the dependencies we have but we need something that turns the spans into ints, longs, datetimes and so on

@JanEggers
Copy link
Author

@JoshClose
Copy link
Owner

Ha. I completely forgot about the converters needing a TryParse overload that takes in Span. Oops.

@JoshClose
Copy link
Owner

JoshClose commented Jul 13, 2021

Maybe in the case of older frameworks the converters can do a ToString.

@JanEggers
Copy link
Author

jep that will always work

@JoshClose
Copy link
Owner

When I get some time I'll play around with this idea and see whether I want the parser to return only Span or have another method. I see the parser as mostly an internal thing. Changing all the converters would break any custom converter people have created, but at minimum they could just do a ToString and it should be the same. I'd have to check how this would all work with mapping Convert.

@Rand-Random
Copy link

Rand-Random commented Mar 3, 2023

I am currently reading a large CSV file (~8 million records) and am analyzing with dotTrace (jetbrains) performance bottle necks, while looking I found this performance metrics
image

after a google search, I came across this PR, and would like to ask if there is any progress on this front?

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.

less allocations when reading with records
3 participants