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.

Sign up for GitHub

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

Jump to bottom

Close cursor early in calculateFolderSize #13752

Merged
merged 1 commit into from
Jan 29, 2015
Merged

Close cursor early in calculateFolderSize #13752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/private/files/cache/cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,7 @@ public function calculateFolderSize($path, $entry = null) {
'WHERE `parent` = ? AND `storage` = ?';
$result = \OC_DB::executeAudited($sql, array($id, $this->getNumericStorageId()));
if ($row = $result->fetchRow()) {
$result->closeCursor();
Copy link

Choose a reason for hiding this comment

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

Isn't it slightly better to move this line outside the if check, add an additional statement $resultRow = $result->fetchRow(); and change the if-statement to if ($row = $resultRow) { ?

Copy link
Contributor

Choose a reason for hiding this comment

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

or simply if($resultRow) ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be slightly better, yes. But I think the current approach is still acceptable.

I preferred investing my time in testing this bad bug and making sure it work instead of making the code perfect. (note: still busy testing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to make the change yourself if you think that's very important.

Copy link

Choose a reason for hiding this comment

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

@PVince81
I am probably not up to it, since I don't have a test environment and am only following the changes. The advantage of my proposed approach is that the connection to the database is freed at the earliest point and additionally in a single place. This might prevent regressions, once other parts of that function are touched or additional cases added to the if statement.

If you still think I should provide a commit without me having a test environment and practically zero php-experience, I would provide it. Should I then create a new PR with a feature branch having your commit against HEAD of master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the performance gain for this change will be that much, but anyway if you'd like to contribute, you should fork the core repo and then submit PRs of a feature branch to core master.

You are saying you have zero PHP experience and yet are making suggestions to improve PHP code ? 😉
Anyway, there's always opportunities to learn.

If you are interested in contributing code/changes, I suggest to setup a dev env http://doc.owncloud.org/server/7.0/developer_manual/general/devenv.html
Then fork the project and submit a PR to master from a feature branch.

Please note that we are currently in the final phase of 8.0 release (so a bit of a rush to finish it), so any non-critical PRs will have to wait for 8.1. 😄

Copy link

Choose a reason for hiding this comment

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

Thanks for elaborating. I knew most about it, yet primarily wanted to know, if you consider this kind of change too much of nitpicking. Certainly no rush is needed for this kind of change, so don't bother with me right now.
No php, but isn't it all the same at an abstract level? ;-)

list($sum, $min, $unencryptedSum) = array_values($row);
$sum = 0 + $sum;
$min = 0 + $min;
Expand All @@ -618,6 +619,8 @@ public function calculateFolderSize($path, $entry = null) {
if ($totalSize !== -1 and $unencryptedSum > 0) {
$totalSize = $unencryptedSum;
}
} else {
$result->closeCursor();
Copy link

Choose a reason for hiding this comment

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

The else branch could be removed (see first note).

}
}
return $totalSize;
Expand Down
1 change: 1 addition & 0 deletions lib/private/files/cache/homecache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public function calculateFolderSize($path, $entry = null) {
'WHERE `parent` = ? AND `storage` = ? AND `size` >= 0';
$result = \OC_DB::executeAudited($sql, array($id, $this->getNumericStorageId()));
if ($row = $result->fetchRow()) {
$result->closeCursor();
list($sum, $unencryptedSum) = array_values($row);
$totalSize = 0 + $sum;
$unencryptedSize = 0 + $unencryptedSum;
Expand Down