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

Verify whether value is already normalized #13223

Merged
merged 1 commit into from Jan 10, 2015

Conversation

Projects
None yet
6 participants
@LukasReschke
Copy link
Member

LukasReschke commented Jan 9, 2015

Apparently normalizer_normalize is not verifying itself whether the string needs to be converted or not. Or does it at least not very performantly.

This simple change leads to a 4% performance gain on the processing of normalizeUnicode. Since this method is called quite often (i.e. for every file path) this has actually a measurable impact. For examples searches are now 200ms faster on my machine. Still not perfect but way to go.

Part of #13221

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Jan 9, 2015

👍 merge for oC 8?

@MorrisJobke

View changes

lib/private/util.php Outdated
@@ -1274,14 +1274,17 @@ public static function clearOpcodeCache() {
* @return bool|string
*/
public static function normalizeUnicode($value) {
if(normalizer_is_normalized($value)) {

This comment has been minimized.

@MorrisJobke
@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 10, 2015

@karlitschek @DeepDiver1975 Please comment on "8.0 or 8.1?" ;)

@LukasReschke LukasReschke force-pushed the optimize-normalize-unicode branch 3 times, most recently Jan 10, 2015

Verify whether value is already normalized
Apparently `normalizer_normalize` is not verifying itself whether the string needs to be converted or not. Or does it at least not very performantly.

This simple change leads to a 4% performance gain on the processing of normalizeUnicode. Since this method is called quite often (i.e. for every file path) this has actually a measurable impact. For examples searches are now 200ms faster on my machine. Still not perfect but way to go.

Part of #13221

@LukasReschke LukasReschke force-pushed the optimize-normalize-unicode branch to e80ece9 Jan 10, 2015

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 10, 2015

The inspection completed: 2 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/6307/
👍 Test PASSed. 👍

@karlitschek

This comment has been minimized.

Copy link
Member

karlitschek commented Jan 10, 2015

i think this looks very good. 👍 for 8.0 from me

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

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

Merge pull request #13223 from owncloud/optimize-normalize-unicode
Verify whether value is already normalized

@MorrisJobke MorrisJobke merged commit c259733 into master Jan 10, 2015

1 check passed

default Merged build finished.
Details

@MorrisJobke MorrisJobke deleted the optimize-normalize-unicode branch Jan 10, 2015

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