Skip to content
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

Performance opt on scandir() usage #5511

Merged
merged 1 commit into from Dec 14, 2016
Merged

Performance opt on scandir() usage #5511

merged 1 commit into from Dec 14, 2016

Conversation

ghost
Copy link

@ghost ghost commented Apr 29, 2016

Questions Answers
Branch? 1.6.1.x
Description? optimize performances in clearCache() function
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? This PR has no real impact. However, you can still check there is no regression by verifying if the folder /theme/cache is empty after a clear cache.
  • avoid tests inside the loop
  • avoid to double skip of index.php

- avoid tests inside the loop
- avoid to double skip of `index.php`
@xBorderie
Copy link
Contributor

This would probably be also interesting on PS 1.7. What do you think, @jocel1 ?

$css_url = str_replace(_PS_ROOT_DIR_, '', $protocol_link.Tools::getMediaServer('').$cache_path.DIRECTORY_SEPARATOR.$file);
$splitted_css[$css_url] = 'all';
}
foreach (array_diff(scandir($cache_path), array('..', '.')) as $file) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not computing the array_diff outside of the foreach ?

Copy link
Author

Choose a reason for hiding this comment

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

Are you sure it would be faster? I can't find any doc about this (not having assigned the array to any variables .. )

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue with computing the array_diff within the foreach is that you make the code computing the same value in every loop. By taking the array_diff computation out of the loop and into a variable, you gain some valuable microseconds, I'm sure -- moreover because you computation performs a scandir, which touches upon the filesystem and therefore waaaaaay slower than any regular array creation :)

Copy link
Author

@ghost ghost May 6, 2016

Choose a reason for hiding this comment

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

My dubt was about to consider or not the (...) part of the loop to be
"inside" it
.. in other words I was not sure that this part of code is evaluated more
than one time ..

I'll do some tests to prove or negate this fact asap

Copy link
Author

Choose a reason for hiding this comment

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

Please see this poc

Copy link
Contributor

Choose a reason for hiding this comment

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

oups you're right, I mixed up with for / while cases. Seems it's also bad to review code on friday :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Always nice to have a reminder, thanks @ZiZuu-store :)

Copy link
Author

Choose a reason for hiding this comment

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

eheh I'm here for this @xBorderie -_^

@ghost
Copy link
Author

ghost commented May 6, 2016

@xBorderie already made a PR for PS1.7 too ;) #5511

@xBorderie
Copy link
Contributor

Ah yes, I see it: #5510. Thanks!
Let's discuss your ideas in this ticket, then please update that other PR accordingly :)

@xBorderie
Copy link
Contributor

Well, seems good to go. Waiting for a proper review by the dev, then both this PR and #5510 should be in :)

@xBorderie xBorderie added Improvement Type: Improvement waiting for code review Waiting for QA Status: action required, waiting for test feedback labels May 10, 2016
@xBorderie xBorderie added this to the 1.6.1.6 milestone May 10, 2016
@Shudrum Shudrum removed this from the 1.6.1.6 milestone May 18, 2016
@ghost
Copy link
Author

ghost commented Nov 14, 2016

up :)

@xBorderie
Copy link
Contributor

Since the develop version of this (#5510] has been merged, I guess this one should be too. Adding this PR to the next 1.6 milestone, then.

@xBorderie xBorderie added this to the 1.6.1.11 milestone Dec 9, 2016
@maximebiloe
Copy link
Contributor

Thank you @ZiZuu-store

@maximebiloe maximebiloe changed the title [*] CORE : performance opt on scandir() usage Performance opt on scandir() usage Dec 14, 2016
@maximebiloe maximebiloe merged commit 119eb8b into PrestaShop:1.6.1.x Dec 14, 2016
@ghost ghost deleted the patch-9 branch December 14, 2016 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Type: Improvement Waiting for QA Status: action required, waiting for test feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants