Skip to content

Conversation

@Glandos
Copy link
Contributor

@Glandos Glandos commented Nov 1, 2022

getCachedPreview used to call getFile, and this calls getDirectoryListing (or underlying function that list directory) to find the file. This was done for every preview spec. Now, this is done only once at the beginning of the loop, and the array is just iterated when needed to find the correct entry.

I have a slow machine, and the result of preview:generate-all from Preview Generator on an image directory containing 1474 images went from 159 seconds to 33 seconds when all preview images already exist. When preview must be generated, this is barely efficient, since image operation are much more expensive.

But this is really needed for running re-generation checks.

@szaimen szaimen added the 3. to review Waiting for reviews label Nov 1, 2022
@szaimen szaimen added this to the Nextcloud 26 milestone Nov 1, 2022
@szaimen szaimen requested review from CarlSchwan, icewind1991 and skjnldsv and removed request for CarlSchwan November 1, 2022 17:43
@skjnldsv
Copy link
Member

skjnldsv commented Nov 3, 2022

Phpunit failure

There was 1 error:
220 |  
221 | 1) Test\Preview\GeneratorTest::testGetCachedPreview
222 | TypeError: Argument 2 passed to OC\Preview\Generator::generatePreview() must implement interface OCP\IImage, null given, called in /drone/src/lib/private/Preview/Generator.php on line 210
223 |  
224 | /drone/src/lib/private/Preview/Generator.php:511
225 | /drone/src/lib/private/Preview/Generator.php:210
226 | /drone/src/lib/private/Preview/Generator.php:114
227 | /drone/src/tests/lib/Preview/GeneratorTest.php:134

@skjnldsv skjnldsv added 2. developing Work in progress performance 🚀 enhancement and removed 3. to review Waiting for reviews labels Nov 3, 2022
getCachedPreview used to call `getFile`, and this calls `getDirectoryListing` (or underlying function that list directory) to find the file. This was done for every preview spec.
Now, this is done only once at the beginning of the loop, and the array is just iterated when needed to find the correct entry.

Signed-off-by: Glandos <bugs-github@antipoul.fr>
Signed-off-by: Glandos <bugs-github@antipoul.fr>
Signed-off-by: Glandos <bugs-github@antipoul.fr>
Signed-off-by: Glandos <bugs-github@antipoul.fr>
Signed-off-by: Glandos <bugs-github@antipoul.fr>
getDirectoryListing should return all files
getFile is not called anymore

Signed-off-by: Glandos <bugs-github@antipoul.fr>
@Glandos
Copy link
Contributor Author

Glandos commented Nov 3, 2022

At last. I spent 2 hours on the wrong path, thinking that helper->getImage() was returning null where in fact, it shouldn't have been called at all.

I've adapted tests.

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Nov 4, 2022
@Glandos
Copy link
Contributor Author

Glandos commented Nov 5, 2022

I didn't really try to run acceptance tests locally, but by reading the log, it seems that this is just a timeout on a normal rendered element.

@Glandos
Copy link
Contributor Author

Glandos commented Nov 13, 2022

Superseeded by #35129 which also rebased after #18210 was merged.

@Glandos Glandos closed this Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement performance 🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants