Skip to content

Make LargeFileHelper.php faster by avoiding execs as much as possible - #9490

Merged
rullzer merged 1 commit into
nextcloud:masterfrom
marco44:faster_large_filehelper_32bits
May 18, 2018
Merged

Make LargeFileHelper.php faster by avoiding execs as much as possible#9490
rullzer merged 1 commit into
nextcloud:masterfrom
marco44:faster_large_filehelper_32bits

Conversation

@marco44

Copy link
Copy Markdown
Contributor

Should fix#8405, as per dissussed in the issue

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code makes sense 👍

@MorrisJobke

Copy link
Copy Markdown
Member

@marco44 Could you add a Signed-Off message to your commit. For more details see https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work

Comment threadlib/private/LargeFileHelper.php Outdated
if (strpos($os, 'linux') !== false) {
return $this->exec('stat -c %Y ' . escapeshellarg($fullPath));
try {
$result= filemtime($fullPath);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you adjust the code style here so we have a space between the variable name and the =?

Comment threadlib/private/LargeFileHelper.php Outdated
try {
$result= filemtime($fullPath);
} catch (\Exception $e) {
$result=-1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here.

Comment threadlib/private/LargeFileHelper.php Outdated
return (float) sprintf('%u', $result);
}
return $result;
return $result;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

1 space is enough. 😉

@marco44
marco44force-pushed the faster_large_filehelper_32bits branch from ca84a16 to 5926218CompareMay 17, 2018 12:20
@codecov

codecovBot commented May 17, 2018

Copy link
Copy Markdown

Codecov Report

Merging #9490 into master will decrease coverage by 44.45%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #9490 +/- ##
============================================
- Coverage 51.12% 6.67% -44.46% - Complexity 25724 25726 +2 
============================================
Files 1574 1641 +67 Lines 88510 96457 +7947 Branches 0 1393 +1393 ============================================
- Hits 45252 6436 -38816 - Misses 43258 90021 +46763
Impacted FilesCoverage ΔComplexity Δ
lib/private/LargeFileHelper.php0% <0%> (-65.08%)28 <0> (+2)
apps/comments/lib/AppInfo/Application.php0% <0%> (-100%)1% <0%> (ø)
lib/private/Color.php0% <0%> (-100%)1% <0%> (ø)
apps/files/lib/Activity/Settings/FileFavorite.php0% <0%> (-100%)8% <0%> (ø)
lib/private/Comments/ManagerFactory.php0% <0%> (-100%)2% <0%> (ø)
lib/private/SystemTag/ManagerFactory.php0% <0%> (-100%)3% <0%> (ø)
...eLimiting/Exception/RateLimitExceededException.php0% <0%> (-100%)1% <0%> (ø)
lib/private/Files/Cache/Wrapper/JailPropagator.php0% <0%> (-100%)1% <0%> (ø)
lib/private/TagManager.php0% <0%> (-100%)4% <0%> (ø)
...eware/Security/Exceptions/NotLoggedInException.php0% <0%> (-100%)1% <0%> (ø)
... and 946 more

@marco44

marco44 commented May 17, 2018

Copy link
Copy Markdown
ContributorAuthor

Corrected all your remarks and amended the commit so the signed-off is there

Signed-off-by: Marc Cousin <cousinmarc@gmail.com>
@marco44
marco44force-pushed the faster_large_filehelper_32bits branch from 5926218 to 32fd091CompareMay 17, 2018 12:28
@MorrisJobke

Copy link
Copy Markdown
Member

Best to be reviewed in the whitespace cleaned diff: https://github.com/nextcloud/server/pull/9490/files?w=1

@juliusknorrjuliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 18, 2018
@juliusknorr

Copy link
Copy Markdown
Member

CI failure seems unrelated

@MorrisJobke

Copy link
Copy Markdown
Member

CI failure seems unrelated

Correct.

I would still like to have feedback from @icewind1991 on this one.

@juliusknorrjuliusknorr added 3. to review Waiting for reviews and removed 4. to release Ready to be released and/or waiting for tests to finish labels May 18, 2018

@icewind1991icewind1991 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks fine

@juliusknorrjuliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 18, 2018
@rullzer
rullzer merged commit 03a6f8e into nextcloud:masterMay 18, 2018
@MorrisJobke

Copy link
Copy Markdown
Member

Backport in #9656

@icewind1991 I guess this backport makes sense, right?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finish

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stat is being launched for every file that is scanned.

5 participants

@marco44@MorrisJobke@juliusknorr@icewind1991@rullzer