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

NaturalSort performance improvements #13511

Merged
merged 2 commits into from Feb 9, 2015

Conversation

Projects
None yet
6 participants
@Xenopathic
Copy link
Member

Xenopathic commented Jan 20, 2015

@Xenopathic Xenopathic added performance and removed performance labels Jan 20, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 20, 2015

What data did you use for testing? – Has that an impact on #13434: "We waste more than 30 seconds on sorting and comparing the file names" ?

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Jan 20, 2015

@LukasReschke ehh, I just copied all the files from my /etc into a folder readable by oC, then used that. It comes out to 2442 files, with a variety of file names (I wanted to give the parser a real workout 😄)

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 20, 2015

Let me test this then on my setup later

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Jan 20, 2015

@LukasReschke how did your testing go?

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 20, 2015

@Xenopathic Later !== "in 7 hours" ;-)

@Xenopathic Xenopathic force-pushed the naturalsort_speeeeeed branch 3 times, most recently to 47bd54b Jan 21, 2015

@Xenopathic Xenopathic changed the title Use isset() instead of strlen() in NaturalSort NaturalSort performance improvements Jan 21, 2015

$aChunk = $aa[$x];
$bChunk = $bb[$x];
if ($aChunk !== $bChunk) {
if (is_numeric($aChunk) && is_numeric($bChunk)) {
if ($aChunk[0] >= '0' && $aChunk[0] <= '9' && $bChunk[0] >= '0' && $bChunk[0] <= '9') {

This comment has been minimized.

@LukasReschke

LukasReschke Jan 22, 2015

Member

Can you please document what the '' magic does here? – For example in a comment above it?

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Jan 22, 2015

On my instance with having 20k files in a single folder this leads to a speedup of 4 seconds when accessing the file list.

@Xenopathic

This comment has been minimized.

Copy link
Member Author

Xenopathic commented Jan 22, 2015

Just pushed a second commit, related (not a NaturalSort improvement but speeds up list.php by another 5%).

graph

} else {
return $this->getMimetype() === 'httpd/unix-directory' ? self::TYPE_FOLDER : self::TYPE_FILE;
if (!isset($this->data['type'])) {
$this->data['type'] = $this->getMimetype() === 'httpd/unix-directory' ? self::TYPE_FOLDER : self::TYPE_FILE;

This comment has been minimized.

@PVince81

PVince81 Feb 9, 2015

Member

Can you add brackets around $this->getMimeType() === 'httpd/unix-directory' for readability ?

If you'd forget them in JS the behavior would be ('httpd/unix-directory')? ... : ... which is wrong.
Better be sure 😄

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Feb 9, 2015

@Xenopathic please fix the comment above. Other than that the code looks good.

Robin McCorkell added some commits Jan 20, 2015

Robin McCorkell
Performance improvements for NaturalSort
A combination of using isset() instead of count() or strlen(), caching the
chunkify function, and replacing is_numeric() with some comparisons

@Xenopathic Xenopathic force-pushed the naturalsort_speeeeeed branch from 1703be0 to a79757b Feb 9, 2015

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Feb 9, 2015

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

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Feb 9, 2015

Nice improvement, still works 👍

@LukasReschke

This comment has been minimized.

Copy link
Member

LukasReschke commented Feb 9, 2015

Agreed  – let's get this in. 👍

LukasReschke added a commit that referenced this pull request Feb 9, 2015

Merge pull request #13511 from owncloud/naturalsort_speeeeeed
NaturalSort performance improvements

@LukasReschke LukasReschke merged commit 74de345 into master Feb 9, 2015

1 check passed

default Merged build finished.
Details

@LukasReschke LukasReschke deleted the naturalsort_speeeeeed branch Feb 9, 2015

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Feb 9, 2015

The inspection completed: No issues found

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