Skip to content

Conversation

@miaulalala
Copy link
Contributor

@miaulalala miaulalala commented Jul 24, 2024

Summary

Blackfire profiling comparison for an update query:
image

Checklist

@miaulalala miaulalala added 2. developing Work in progress feature: caldav Related to CalDAV internals labels Jul 24, 2024
@miaulalala miaulalala added this to the Nextcloud 30 milestone Jul 24, 2024
@miaulalala miaulalala self-assigned this Jul 24, 2024
@miaulalala miaulalala marked this pull request as draft July 24, 2024 14:36
@miaulalala miaulalala force-pushed the feat/add-delta-sync-to-subscription-calendars branch from b9d52df to 2e504d8 Compare August 5, 2024 14:34
foreach (array_chunk($calendarObjectUris, 1000) as $chunk) {
$query = $this->db->getQueryBuilder();
$query->delete('calendarchanges')
->where($query->expr()->eq('calendarid', $query->createNamedParameter($subscriptionId)))

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\DAV\CalDAV\CalDavBackend::moveCalendar does not have a return type, expecting void
->andWhere($query->expr()->eq('calendartype', $query->createNamedParameter(self::CALENDAR_TYPE_SUBSCRIPTION)))
->andWhere($query->expr()->in('uri', $query->createNamedParameter($chunk, IQueryBuilder::PARAM_STR_ARRAY), IQueryBuilder::PARAM_STR_ARRAY))
->executeStatement();
}

Check notice

Code scanning / Psalm

RiskyTruthyFalsyComparison

Operand of type null|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead.
@miaulalala miaulalala marked this pull request as ready for review August 7, 2024 16:08
@miaulalala miaulalala added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 7, 2024
@Altahrim Altahrim mentioned this pull request Aug 8, 2024
@miaulalala miaulalala force-pushed the feat/add-delta-sync-to-subscription-calendars branch from 7e7c4ae to e0b4627 Compare August 8, 2024 12:36
@ChristophWurst
Copy link
Member

What does the blackfire comparison look like without md5? Does it get even better?

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

@miaulalala
Copy link
Contributor Author

miaulalala commented Aug 12, 2024

What does the blackfire comparison look like without md5? Does it get even better?

md5 vs strcasecmp:
image

@miaulalala miaulalala force-pushed the feat/add-delta-sync-to-subscription-calendars branch from af04739 to c47650c Compare August 13, 2024 08:56
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks good

:shipit:

@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@miaulalala miaulalala force-pushed the feat/add-delta-sync-to-subscription-calendars branch from c47650c to fb94db1 Compare August 13, 2024 11:06
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works.

@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 14, 2024
@st3iny
Copy link
Member

st3iny commented Aug 14, 2024

I ran the freeze check again and we are good.

@st3iny st3iny merged commit b8ec68d into master Aug 14, 2024
@st3iny st3iny deleted the feat/add-delta-sync-to-subscription-calendars branch August 14, 2024 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish feature: caldav Related to CalDAV internals performance 🚀

Projects

Status: ☑️ Done

Development

Successfully merging this pull request may close these issues.

Use Delta for subscription calendars instead of deleting and reinserting all events

4 participants