-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Optimize PROPFIND having CommentsApp enabled by default #27339
Conversation
@mrow4a, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @tcitworld to be potential reviewers. |
EDIT: Updated in main post |
$this->numberOfCommentsForNodes[$folderNodeID] = 0; | ||
|
||
// Get IDs for all children of the parent folder | ||
$children = $folderNode->getDirectoryListing(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets exactly the same result as https://github.com/owncloud/core/blob/master/apps/dav/lib/Connector/Sabre/SharesPlugin.php#L157, so need to be cached, otherwise it is huge overhead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e5946ba
to
4e4e05e
Compare
@DeepDiver1975 @PVince81 New Benchmark in the top post.. Seems comments app will be fixed now. However I want to continue with caching.. I cannot stand it like that :> |
lib/private/Comments/Manager.php
Outdated
* AND CRM.marker_datetime > C.creation_timestamp) | ||
* GROUP BY C.object_id; | ||
* | ||
* @param $objectType string the object type, e.g. 'files' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
@param string $objectType the object type, e.g. 'files'
lib/private/Comments/Manager.php
Outdated
* GROUP BY C.object_id; | ||
* | ||
* @param $objectType string the object type, e.g. 'files' | ||
* @param int[] NodeIDs that may be returned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phpdoc
tests/lib/Comments/ManagerTest.php
Outdated
@@ -293,6 +303,62 @@ public function testGetNumberOfCommentsForObject() { | |||
$this->assertSame($amount, 4); | |||
} | |||
|
|||
public function testGetNumberOfUnreadCommentsForNodes() { | |||
$manager = $this->getManager(); | |||
$user1 = $this->createMock('\OCP\IUser'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better use
IUser::class
76d2686
to
4e84cd2
Compare
@butonic Jorn, do you know which part of the querybuilder for ORACLE could cause error below in core/lib/private/Comments/Manager.php Line 389 in 4e84cd2
I have problems spotting an issue, @DeepDiver1975 says it is |
It seens object_id is not always escaped. |
Try using an alias for c.object_id. Especially in The count function. |
9488209
to
53bbd3e
Compare
@PVince81 @DeepDiver1975 Covered will unit tests and checked manually. To review |
C and CRM are not a problem, as long as they are consistently escaped or not escaped. In contrast to mysql, postgres or sqlite oracle uppercases unquoted identifiers. |
lib/private/Comments/Manager.php
Outdated
$objectIdChunks = array_chunk($objectIds, 100); | ||
foreach ($objectIdChunks as $objectIdChunk) { | ||
// Fetch only records from oc_comments which are in specified int[] NodeIDs array and satisfy specified $objectType | ||
$qbMain->selectAlias('object_id', 'id')->selectAlias($qbMain->createFunction('COUNT(object_id)'), 'count') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>createFunction('COUNT(id)'),
you created an alias. try using it in the count
53bbd3e
to
92516d6
Compare
The mysql docker seems dead? |
92516d6
to
4689863
Compare
@DeepDiver1975 @PVince81 Tests are passing, what do we do with it? Travis? |
4689863
to
044f51f
Compare
@DeepDiver1975 @PVince81 This PR is ready from my side. All checks passing. |
de9fd29
to
260f724
Compare
I haven't tested the code but it looks fine. |
Do we have behavioural tests for comments? Better AI test it @jvillafanez @SamuAlfageme @DeepDiver1975 |
e807d9a
to
9c7194c
Compare
…quired data in one query instead of iterative queries.
9c7194c
to
be4309a
Compare
@jvillafanez Any new input on this PR? Does CommentsApp have integration tests? |
@mrow4a yes:
|
Looks like the comments integration tests are only for the comments Webdav endpoint but not for the files endpoint. Tests are missing for the property "oc:comments-count", "oc:comments-unread", "oc:comments-href". @mrow4a please work with @SergioBertolinSG to get some tests for this added. You can run the integration tests with these commands:
(you can also add line number of the scenario "comments.feature:191". |
No more changes from my side. |
@SergioBertolinSG is working on comments tests separately, merging this. 👍 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
The idea here is to reduce number of O(n) queries coming from each PROPFIND in the web browser for instance with enabled comments app (this is by default).
Prefetching required data in one query instead of iterative queries should reduce number of queries to O(1), where N is number of files in the folder.
Query
SELECT object_id, COUNT(object_id) FROM oc_comments C WHERE object_id IN(?) AND object_id NOT IN(SELECT object_id FROM oc_comments_read_markers CRM WHERE C.object_id = CRM.object_id AND CRM.user_id = '?' AND CRM.marker_datetime > C.creation_timestamp) GROUP BY object_id;
does the job very well. Yes, I know it is more OLAP query, but this is what Web Front End is about. This is also most optimal query in our case https://sqlperformance.com/2012/12/t-sql-queries/left-anti-semi-join
Case 1
- 16 queries
- this is worst case because caching ofgetDirectoryListing
is not yet doneCase 2
100 NON-SHARED/SHARED files, just someone uploaded >100 files to the folder
- 200 queries
- this is best case