Bug 79014 - EDS calendar performance
Summary: EDS calendar performance
Alias: None
Product: SyncEvolution
Classification: Unclassified
Component: SyncEvolution (show other bugs)
Version: 1.4
Hardware: Other All
: medium enhancement
Assignee: Patrick Ohly
QA Contact:
Depends on:
Reported: 2014-05-21 12:16 UTC by Patrick Ohly
Modified: 2018-10-13 12:38 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

rebased and squashed enhancements from Renato (6.81 KB, patch)
2014-05-21 12:16 UTC, Patrick Ohly
Details | Splinter Review
fixes regressions from original patch (4.71 KB, patch)
2014-05-21 12:16 UTC, Patrick Ohly
Details | Splinter Review
Fixed timezone cache and remove hardcoded strings (3.01 KB, patch)
2014-07-02 21:40 UTC, renato filho
Details | Splinter Review
Optimize-calendar-events-update-when-possible. (5.52 KB, text/plain)
2014-07-02 21:40 UTC, renato filho

Description Patrick Ohly 2014-05-21 12:16:04 UTC
Created attachment 99502 [details] [review]
rebased and squashed enhancements from Renato

Renato and I discussed some ways to enhance EDS calendar performance during sync. Some of the ideas were coded as patches by Renato, However, regression testing then found some regressions which I fixed.

The patches need to be fixed, cleaned up and submitted again against current master.
Comment 1 Patrick Ohly 2014-05-21 12:16:39 UTC
Created attachment 99503 [details] [review]
fixes regressions from original patch
Comment 2 renato filho 2014-07-02 21:40:11 UTC
Created attachment 102164 [details] [review]
Fixed timezone cache and remove hardcoded strings
Comment 3 renato filho 2014-07-02 21:40:52 UTC
Created attachment 102165 [details]
Comment 4 Patrick Ohly 2014-07-15 06:12:03 UTC
(In reply to comment #3)
> Created attachment 102165 [details]
> Optimize-calendar-events-update-when-possible.

Can you explain why this is using a time zone's display name as key for the cache?
Comment 5 Patrick Ohly 2014-07-21 08:56:13 UTC
I ran a full test without problems after applying the following change. See "ecal" branch.

commit b44b45b74207c2710415fd9d040427bdcefb67d4
Author: Patrick Ohly <patrick.ohly@intel.com>
Date:   Mon Jul 21 01:07:42 2014 -0700

    EDS calendar: cache by TZID
    The display name is not necessarily a unique identifier for the content of a
    time zone. For example, the 1990 version of CET/CEST might be different from
    the 2014 version. Therefore the display name should not be used as identifier.
    The TZID is not necessarily better, but at least the code in
    e-cal-check-timezones.c tries to verify content equality before reusing an
    existing timezone. It also has code which matches time zones based on their
    Olson name embedded in the TZID. If that code does not work well enough
    (for example, see FDO #81590), then it should be enhanced.

The problem with icaltimezone_get_display_name() also was that it's not currently covered by src/syncevo/eds_abi_wrapper.h, which causes compile errors when building binaries for syncevolution.org. This could be fixed, of course.
Comment 6 renato filho 2014-09-11 20:57:09 UTC
Hi Patryck,

Sorry for take so long to reply, I have been very busy these days.

Ok. I am complete lost here I do not understand how this tz names works. 
Just say want you want to me to do to make it acceptable to merge :D. (in last case lets remove the timezone cache patch).

Do you want to me to use tzid as the key for the cache? I am ok with that.

I do not see a big problem if the cache fail and for some reason stores 2 timezones with the same value.

The main reason to have the cache here is to avoid query eds for new timezone every time that you want to insert an event.
The only problem of storing two timezones with the same value is that EDS will do one extra query, and that still better that a query for every insert.
Comment 7 Patrick Ohly 2015-04-01 13:38:19 UTC
Let's look at this again...

What I'd like to understand better is which timezones this patch caches. Some examples would be nice.
Comment 8 renato filho 2015-04-07 02:14:34 UTC
I was not able to reproduce the timezone problem. We can ignore the timezone optimizations and use the others patches.
Comment 9 Patrick Ohly 2015-04-08 06:54:56 UTC
(In reply to renato filho from comment #8)
> I was not able to reproduce the timezone problem. We can ignore the timezone
> optimizations and use the others patches.

In the future, remember to include such information in the commit message or the source code itself. I still think that the patch has some merit, but without specific examples it is hard to determine how often it is useful and thus whether the added complexity is worth it.

My guess is that it is relevant for events created by Exchange, but I haven't tried it. I might have a look myself.
Comment 10 GitLab Migration User 2018-10-13 12:38:49 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/SyncEvolution/syncevolution/issues/16.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.