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

Implement an basic cache for faces.. Issue #193 #194

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matiasdelellis
Copy link
Owner

Please @Petahh

Small attempt to improve your report.. They are not yet pre-generated, but you can verify that once generated, the load of your server should be lighter to show a complete view.

@matiasdelellis
Copy link
Owner Author

@stalker314314
I add that they are pregenerated after saving the faces to the database?
However, it worries me that it would be a much larger patch than it seemed..
I mean, I have no problem pregenerating it -Compared to the discovery process, it must be much faster..-, but I don't want to have to eliminate it every time something happens with an image/face, ..

@matiasdelellis
Copy link
Owner Author

Note: Nextcloud never eliminates the preview cache ... and that cache would be much bigger ... Can we live with this?

@Petahh
Copy link

Petahh commented Nov 30, 2019

I think only problem is initial pre-generation. If you have many pictures. Maybe could be part of the background job we run to analyze and cluster faces?

@stalker314314
Copy link
Collaborator

I can live if you want to implement this. I think it will be super-hard to nail this right, with all needed cache evictions and so on. But if you want to try it - go for it!

I am more wondering is this problem we need to solve? Thumbs are 32x32, right? I think that Thumbnail Generator app is already doing some thumbnail generation, and even default settings would suite us, right? (32x32 should be one of default sizes for squareSizes) Then, that app takes care of all gory details of what it is doing best - thumbnail generation, and we take care of what we are best:) We should add wiki page explaining and pointing people to Thumbnail generator in FAQ, but that's it - preview generation should not be our job and we should not be wasting time on problem of thumbnail generator, cache population, cache evictions... That's my 2c. If Thumbnail Generator is not helping, we can discuss is this patch "worth it", to say it like that:)

@matiasdelellis
Copy link
Owner Author

Hi both,

I think only problem is initial pre-generation. If you have many pictures. Maybe could be part of the background job we run to analyze and cluster faces?

I agree, maybe do the pre-generation, but until we have an easy cleaning this will not be merged..

I can live if you want to implement this. I think it will be super-hard to nail this right, with all needed cache evictions and so on. But if you want to try it - go for it!

As I said, generate them is easy, but I don't want to deal with cleaning, it must be something more or less automatic.
The implementation of the garbage collector is simple, but it does not work due it depends on the modification date, and not on the last access date. IAppData storage is SimpleFS, that does not support touch to update the modification date and therefore it is a dead end .. 😞

I am more wondering is this problem we need to solve?

If it were just a performance problem, I wouldn't worry about fixing it. but the problem in #193 is that the number of calls in parallel to get the faces breaks the server..

Perhaps it is just an Apache problem, which should not accept so many queries in parallel, and should be limit the threads. I do not know. 😕

Thumbs are 32x32, right? I think that Thumbnail Generator app is already doing some thumbnail generation, and even default settings would suite us, right? (32x32 should be one of default sizes for squareSizes) Then, that app takes care of all gory details of what it is doing best - thumbnail generation, and we take care of what we are best:) We should add wiki page explaining and pointing people to Thumbnail generator in FAQ, but that's it - preview generation should not be our job and we should not be wasting time on problem of thumbnail generator, cache population, cache evictions... That's my 2c. If Thumbnail Generator is not helping, we can discuss is this patch "worth it", to say it like that:)

The previewgenerator app create the complete thumbnail of files, and you can indicate the size, but not the position (Faces position..), or name of file... It does not help us ..

@Biont
Copy link

Biont commented Jan 7, 2020

Would it not help immensely already if you set appropriate headers for client side caching?

$dataDisplayResponse->cacheFor($days * 24 * 60 * 60);
$dataDisplayResponse->addHeader('Pragma', 'cache');
return $dataDisplayResponse;

@matiasdelellis
Copy link
Owner Author

Hi @Biont
Thanks for the suggestion, it's something that I need to fix, but I wasn't convinced to cache all the faces in the browser.. but if it improve the behavior I can accept a patch with this change .. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants