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

Reduce drastically allocation rate when parsing CSS Colors. #239

Merged
merged 1 commit into from
Nov 27, 2022

Conversation

ciplogic
Copy link
Contributor

It reduces allocation rate by creating a span like construct, so when moving to .Net (aka Core), this code would have most of functionality to port to it.

@paulushub
Copy link
Contributor

Thanks for the contribution. Using conditional compilation, can you add the .Net Core ports?

@paulushub paulushub merged commit 541b01c into ElinamLLC:master Nov 27, 2022
@ciplogic
Copy link
Contributor Author

ciplogic commented Nov 28, 2022

Hello, I have a question Paul and I will make it over this week (sorry to not answer yesterday, but I was with my family).

So let' me understand you as your comment can be addressed in 2 ways:

  • add reference to System.Memory (which has access to Span), but it doesn't support I think anything older than 4.6 or 4.7) and add a Span usage implementation
  • add a .Net Core only Span implementation with no external references.

Which one would it be more convenient?

As for a timeline: I will contribute it over this week time I assume, as I do it over my free time.

As for this Merge Request: works in Net Core unmodified, just it doesn't get the Span related JIT improvements.

@paulushub
Copy link
Contributor

add reference to System.Memory

Yes, it is .NET4.6.1+, .NET-Std 1.1+. 2.0+

add a .Net Core only Span implementation with no external references.
Yes, the implementation is up to you.

As for a timeline: I will contribute it over this week time I assume, as I do it over my free time.

Anytime is fine. I have committed the current implementation and could make it available in Nuget package if you need it.

@ciplogic
Copy link
Contributor Author

Thank you Paul!

My company is using SharpVectors and I thought to contribute back (I am not getting any paycheck out of it), as this is an ethical way to do it.

As for NuGet bump, I will want to profile a bit more the code, and see if I can see other perf issues (if any I can see with my time), and maybe in 1 month time frame, I can ask you for a bump.

So maybe around New Year's time (of course, if you are in holidays, it can be after holidays).

Sayonara!

@ciplogic
Copy link
Contributor Author

ciplogic commented Dec 7, 2022

I tried to port to Span and I noticed that are few parts I cannot do it, especially Split() as it requires heap creation of slices.

I also profiled more, and right now the memory consumption is around Regex, so even if I could circumvent and find a nice way to do the spans, it may not worth the effort.

@paulushub
Copy link
Contributor

@ciplogic Thanks for the efforts.

I also profiled more, and right now the memory consumption is around Regex...

The extensive use of the Regex is one of the problems inherited from SharpVectorGraphics. I have started a new project privately to create a new library, but cannot easily find the time - mostly doing Python, Java and C++ stuff.

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.

None yet

2 participants