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

Use isset() instead of strlen() #13236

Merged
merged 1 commit into from Jan 10, 2015

Conversation

Projects
None yet
5 participants
@LukasReschke
Copy link
Member

LukasReschke commented Jan 10, 2015

Isset is a native language construct and thus A LOT faster than using strlen() (5x - 40x depending on input)

On my local machine this leads to a per average 20% performance gain, for about 1 million processed paths on my local machine this means a 1s faster processing time. Considering that this function will be called a lot for every file operation this makes a noticeable difference.

@DeepDiver1975 @karlitschek @MorrisJobke

@MorrisJobke

View changes

lib/private/files/view.php Outdated
@@ -1276,10 +1276,10 @@ public function getPath($id) {
return null;
}

private function assertPathLength($path) {
public function assertPathLength($path) {

This comment has been minimized.

@MorrisJobke

MorrisJobke Jan 10, 2015

Member

Why is this now public?

This comment has been minimized.

@LukasReschke

LukasReschke Jan 10, 2015

Author Member

Ah – yes … - Was testing it by directly accessing … - Stupid me forgot to change that back after that again ;-)

This comment has been minimized.

@LukasReschke

LukasReschke Jan 10, 2015

Author Member

Adjusted. THX for catching.

@LukasReschke LukasReschke force-pushed the use-isset-for-performance branch Jan 10, 2015

@MorrisJobke

View changes

lib/private/files/view.php Outdated
@@ -1278,8 +1278,8 @@ public function getPath($id) {

private function assertPathLength($path) {
$maxLen = min(PHP_MAXPATHLEN, 4000);
$pathLen = strlen($path);
if ($pathLen > $maxLen) {
if(isset($path[$maxLen])) {

This comment has been minimized.

@MorrisJobke

MorrisJobke Jan 10, 2015

Member

Nice would be a commend why you do this. Just a comment that this will check for the string length would enhance the code quality and readability a lot

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 10, 2015

Beside my comment 👍

I played a bit around and couldn't notice an error in the logs.

@LukasReschke LukasReschke force-pushed the use-isset-for-performance branch Jan 10, 2015

Use isset() instead of strlen()
Isset is a native language construct and thus A LOT faster than using strlen()

On my local machine this leads to a 1s performance gain for about 1 million paths. Considering that this function will be called a lot for every file operation this makes a noticable difference.

@LukasReschke LukasReschke force-pushed the use-isset-for-performance branch to 310424d Jan 10, 2015

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 10, 2015

The inspection completed: 4 new issues

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Jan 10, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/6308/
👍 Test PASSed. 👍

@karlitschek

This comment has been minimized.

Copy link
Member

karlitschek commented Jan 10, 2015

very nice 👍

MorrisJobke added a commit that referenced this pull request Jan 10, 2015

@MorrisJobke MorrisJobke merged commit 8057bc6 into master Jan 10, 2015

1 check passed

default Merged build finished.
Details

@MorrisJobke MorrisJobke deleted the use-isset-for-performance branch Jan 10, 2015

@MorrisJobke MorrisJobke added this to the 8.0-current milestone Jan 10, 2015

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