Skip to content

Conversation

@axunonb
Copy link
Collaborator

@axunonb axunonb commented Jan 18, 2025

  • Update line folding to use StringBuilder and ArrayPool for better performance and memory efficiency.
  • Ensure compliance with RFC 5545 by handling multi-byte characters correctly when folding lines
  • Add System.Buffers package for netstandard2.0 target framework.

Resolves #693

@axunonb axunonb requested a review from minichma January 18, 2025 21:07
- Update line folding to use StringBuilder and ArrayPool<char> for better performance and memory efficiency.
- Ensure compliance with RFC 5545 by handling multi-byte characters correctly.
- Add System.Buffers package for netstandard2.0 target framework.

Resolves ical-org#693

var result = new StringBuilder();
foreach (var v in prop.Values.Where(value => value != null))
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reducing cognitive complexity by splitting up this method

Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

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

Basically LGTM. One finding would be that I think it would be desirable for a serializer to treat the values it operates on as immutable but we modify the TZID property of the period list. Not sure we can change that. I know that's not a new pattern though.

@axunonb
Copy link
Collaborator Author

axunonb commented Jan 20, 2025

Excellent, thanks for pointing this out.
The mentioned code became fully redundant with #684: All timezones of a PeriodList are always the same, and there's nothing left to care for in PropertySerializer when serializing 👍

// Set TzId before ValueType, so that it serializes first
if (firstPeriod != null && !string.IsNullOrEmpty(firstPeriod.TzId) && firstPeriod.TzId != "UTC")
{
periodList.Parameters.Set("TZID", periodList[0].TzId);
}

@axunonb axunonb requested a review from minichma January 20, 2025 22:32
@minichma
Copy link
Collaborator

The mentioned code became fully redundant with #684: All timezones of a PeriodList are always the same, and there's nothing left to care for in PropertySerializer when serializing 👍

Wonderful! It would be good at some point to get rid of the redundant data we have in this regard now. I.e. we have the Properties collection which can hold a TZID proprety, as well as the explicit TzId fields in Period.StartDate.

Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

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

LGTM, just another very small comment

const int takeLimit = 74;
// Use ArrayPool<char> to rent arrays for processing
// Cannot use Span<char> and stackalloc because netstandard2.0 does not support it
var arrayPool = ArrayPool<char>.Shared;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the performance optimization the usage of the ArrayPool could bring but I would usually try to avoid using a global pool here. It increases complexity and could have unexpected side effects. Had similar situations (in other languages though) where some memory sanitizer suddenly complained that the globally allocated memory changed after calling a certain method just because of some global cache. This might not be the case here but given the quite small effect we can expect from the optimization (compared to just allocating a new array) and given the fact that we don't know how/where users of the lib will will use it, I would rather avoid it.

Question is, whether its needed at all. Why not directly append to the result?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

KISS, replaced it

minichma
minichma previously approved these changes Jan 21, 2025
@sonarqubecloud
Copy link

@axunonb axunonb merged commit 5c6ba1a into ical-org:main Jan 21, 2025
4 checks passed
@axunonb axunonb deleted the pr/fold-lines branch January 21, 2025 09:09
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.

Error in line folding algorithm

2 participants