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() #19

Merged
merged 1 commit into from Jan 15, 2015

Conversation

Projects
None yet
2 participants
@LukasReschke
Copy link
Contributor

LukasReschke commented Jan 15, 2015

strlen() is not an op-code while isset() is one. Thus this function was way too slow before and when called very often (e.g. 100k times with long string values) took more than one minute before for us at ownCloud.

This should make the function about 50% faster according to the blackfire.io profiler. (and also reduce the overall memory usage)

A simple test script showed the following changes for me:

screen shot 2015-01-15 at 12 29 44

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 15, 2015

@nickvergessen pointed out to me that we could possibly save even more when optimizing the ord() operations as well. – I'll create another PR for that later.

@@ -87,7 +87,7 @@ public static function getPrefix($str, $length)
}
}
if ($prefixBytes + $charBytes > strlen($str)) {
if(!isset($str[$prefixBytes+$charBytes-1])) {

This comment has been minimized.

@Ocramius

Ocramius Jan 15, 2015

Member

Please fix CS here:

if (! isset($str[$prefixBytes + $charBytes - 1])) {

This comment has been minimized.

@LukasReschke

LukasReschke Jan 15, 2015

Author Contributor

Fixed

Use isset() instead of strlen()
`strlen()` is `O(n)` while `isset()` is `O(1)`. Thus this function was way too slow before and when called very often (e.g. 100k times with long string values) took more than one minute before us at ownCloud.

This should make the function about 50% faster according to the blackfire.io profiler. (and also reduce memory the overall memory usage)

@LukasReschke LukasReschke force-pushed the LukasReschke:replace-strlen branch from 25d06fe to 299974b Jan 15, 2015

@Ocramius Ocramius self-assigned this Jan 15, 2015

@Ocramius

This comment has been minimized.

Copy link
Member

Ocramius commented Jan 15, 2015

Merging, thanks!

Ocramius added a commit that referenced this pull request Jan 15, 2015

@Ocramius Ocramius merged commit 38b2ad8 into zendframework:master Jan 15, 2015

@LukasReschke LukasReschke deleted the LukasReschke:replace-strlen branch Jan 15, 2015

@LukasReschke

This comment has been minimized.

Copy link
Contributor Author

LukasReschke commented Jan 15, 2015

@butonic Would be nice if we could get into our packages

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