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

Strip leading solidus (slash) from TZID if present #204

Closed
ChlorideCull opened this issue Dec 15, 2016 · 1 comment
Closed

Strip leading solidus (slash) from TZID if present #204

ChlorideCull opened this issue Dec 15, 2016 · 1 comment

Comments

@ChlorideCull
Copy link

As you're probably aware, RFC 5545 specifies, for the TZID parameter,

The presence of the SOLIDUS character as a prefix, indicates that
this "TZID" represents a unique ID in a globally defined time zone
registry (when such registry is defined).

Barring the whole "registry isn't defined by the spec thing", performance is greatly improved by stripping a leading slash from the TZID by present, and makes no difference otherwise - the library already treats all TZIDs as global/external and doesn't use defined VTIMEZONE blocks. What it does, is allow TZIDs such as /Europe/Stockholm map cleanly to IANA, allowing an index lookup instead of iterating over all NodaTime IDs, looking for IDs containing the TZID.


For reference when it comes to performance increase, consider the following ICS document.

BEGIN:VCALENDAR
VERSION:2.0
PRODID:HandwrittenStuff
BEGIN:VEVENT
DTSTAMP:20161201T151152Z
DTSTART;TZID=/Europe/Stockholm;TYPE=DATE:20160101
RRULE:FREQ=WEEKLY;BYDAY=TU;INTERVAL=1;UNTIL=20161230T230000Z
UID:UNIQUEIDENTIFIERICOULDNTBEBOTHEREDTOGENERATE
END:VEVENT
END:VCALENDAR

Running GetOccurences from slightly before DTSTAMP to slightly after UNTIL on a calendar object loaded with this document, 100 times, gives a respectable speedup, as seen below in dotTrace.

dotTrace

Note that this was done by adding the check to GetZone in DateUtil. Probably not the correct place to add it, which is why this isn't a pull request.

(Also note that this massively improved ToList performance when converting from Linq objects, as it will QuickSort, making heavy use of Equals, which makes use of GetZone.)

(Sorry for rambling on a bit, it's getting late and I've been stuck trying to optimize parsing ICS documents like this for the last few hours.)

@rianjs
Copy link
Collaborator

rianjs commented Dec 15, 2016

Oh, cool. I didn't know that about the solidus character. In modern date/time programming using a hash-based data structure like a Dictionary, it feels... anachronistic. Prefixing a string with it will just incur garbage collection + memory allocation overhead.

Oh well.

Given the solidus is defined in the spec, I think it makes sense to check for its presence, and snip it off if it's there. DateUtil.GetZone is definitely the code path to do this in. (DateUtil.GetZone is responsible for probably 80% of the 10x speedup of ical.net over the original dday.ical.) This operation doesn't slow down this hot path very much.

I've put in the change, and published another nuget package version. I also started keeping notes about the changes each package version has:

https://github.com/rianjs/ical.net/blob/master/release-notes.md

Nuget version 2.2.24 should be indexed in a few minutes:
https://www.nuget.org/packages/Ical.Net

@rianjs rianjs closed this as completed Dec 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants