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

Don't use php-functions to load cached thumbnails, use the direct path for speed improvement. #94

Open
starchimpsgroup opened this Issue Apr 12, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@starchimpsgroup

starchimpsgroup commented Apr 12, 2016

The current solution works, but it is to slow for big galleries (at least on my server).

@sidler

This comment has been minimized.

Member

sidler commented Apr 12, 2016

In general - yes, you are totally right!
Nevertheless, we're still struggling with finding a good way.

Our current strategy described in a few words:
Image operations are handled by image.php. The Image2 class creates a list of operations per image (queued) and creates a hash-sum for the image-operations. Before processing the operations like cut and resize, the image2 class performs a cache lookup.
If an image is found in the cache - matching the checksum - the image2 class uses fpassthru() to pass the image directly to the browser.
If the image is not found, the operations are applied to the image, the image is saved to the cache and passed to the browser.
So in the best case, the php-impact is "only" generating a cache-sum and passing the cached image.

Nevertheless, we totally agree that a direct access without php would be way better. But - what is the best strategy? Do you have any ideas?
We already thought about a complex mechansim of using htaccess rewrite rules, but the costs on the server-side were equal or even higher then processing the image by php.

Combining the initial image-cache generation with the generation of the gallery page would slow down the first call of the gallery page dramatically - so not really the best solution, isn't it?

While writing this lines another approach comes to my mind:
Hows about generating the image-cachesums along with the gallery page. In addition, the gallery performs a cache lookup: If the image is found in the cache, a "real" src url is returned, so no image.php. if the images is not found in the cache, the image.php is returned as the src attribute, allowing the parallel generation of the images (and therefore the cache warm up).
So at the second generation of the gallery, all srcs should be image.php-less urls. What do you think? Interested in hacking it within a feature branch? I'll help you with the details whenever a question arises.

@jschroeter what do you think about the last approach?

@starchimpsgroup

This comment has been minimized.

starchimpsgroup commented Apr 12, 2016

Hallo Stefan,

danke für die schnelle Antwort, ich habe eine Lösung dazu schon länger im Einsatz, aber ich wollte für zukünftige Updates eine Änderung am Quellcode vermeiden.
Ob mein Ansatz jetzt so Elegant ist, bezweifle ich aber :D

Ich glaube aber, es nährt sich schon deinem Gedankengang an.

Grüße, Christian

class class_scriptlet_imagehelper:
foreach($arrTemp[0] as $intKey => $strSearchString) {
if(isset($arrTemp[4][$intKey]) && $arrTemp[4][$intKey] == ",fixed") {
$objImage = new class_image2();
$objImage->load($arrTemp[1][$intKey]);
$objImage->addOperation(new class_image_scale_and_crop($arrTemp[2][$intKey],$arrTemp[3][$intKey]));
$objImage->addOperation(new class_image_scale(0, 0));
$strFormat = $objImage::getFormatFromFilename($arrTemp[1][$intKey]);
if($objImage->isCached($strFormat)) {
$strContent = uniStrReplace(
$strSearchString,
webpath . $objImage->getCachePath($strFormat),
$strContent
);
} else {
$strContent = uniStrReplace(
$strSearchString,
webpath . "/image.php?image=" . urlencode($arrTemp[1][$intKey]) . "&fixedWidth=" . $arrTemp[2][$intKey] . "&fixedHeight=" . $arrTemp[3][$intKey],
$strContent
);
}
}
else {
$objImage = new class_image2();
$objImage->load($arrTemp[1][$intKey]);
$objImage->addOperation(new class_image_scale_and_crop(0, 0));
$objImage->addOperation(new class_image_scale($arrTemp[2][$intKey], $arrTemp[3][$intKey]));
$strFormat = $objImage::getFormatFromFilename($arrTemp[1][$intKey]);
if($objImage->isCached($strFormat)) {
$strContent = uniStrReplace(
$strSearchString,
webpath . $objImage->getCachePath($strFormat),
$strContent
);
} else {
$strContent = uniStrReplace(
$strSearchString,
webpath . "/image.php?image=" . urlencode($arrTemp[1][$intKey]) . "&maxWidth=" . $arrTemp[2][$intKey] . "&maxHeight=" . $arrTemp[3][$intKey],
$strContent
);
}
}
}

@sidler

This comment has been minimized.

Member

sidler commented Apr 15, 2016

um ehrlich zu sein finde ich die Variante nicht mal schlecht! kannst du das denn bei Gelegenheit mal auf V5 bauen? Sofern du einen Pull-Request aufmachst könnten wir das "git style mäßig" mergen, d.h. der Commit würde dann auch dir gehören - was Dir ja zustehen würde!

@jschroeter

This comment has been minimized.

Member

jschroeter commented Apr 15, 2016

I totally agree, PHP should not be needed to deliver images - it just can't provide the performance which is possible when it's handled by the webserver itself.

As far as I can see your solution does exactly what @sidler described in his second approach. I think it's a pragmatic way, without the disadvantages of requiring mod_rewrite or something similar and having the logic in two places which come with mod_rewrite rules.

Some things to keep in mind:

  • our current image.php solution sends correct HTTP headers for client-side caching of the images. We should check if our rules in the .htaccess apply when delivering the cached images directly and maybe adjust them.
  • the new approach isn't ideal in terms of client-side caching and SEO since the same image is accessible through two URLs. But this can be solved: the image.php could send a 301 redirect header to the final cached image instead of delivering it. This of course leads to a little overhead for the initial user. But the advantage is, that we only have one URL to the cached image, so client-side caching works well and we don't have duplicate content (which I guess is not that critical for images anyway).
  • is a checksum/timestamp of the image included in the cached file name? Just to cover the case of replacing an image via the file manager.
  • because of our page/element cache, the image.php URL will be used not only for the initial user. This is not a problem, just a thing to keep in mind.

I'm looking forward to the pull-request! ;-)

@sidler

This comment has been minimized.

Member

sidler commented Apr 18, 2016

@jschroeter Good point regarding the SEO issues. Imho a 301 redirect to the cached image would be a feasible solution.
Regarding the cache we won't run into problems. The img-urls are (at least in 99% of all cases) replaced by the scriptlet @starchimpsgroup postet. The scriptlet is applied to the content after being loaded from the cache, so caching won't be an issue.

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