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

Non localized pictures devices #46

Closed

Conversation

tacruc
Copy link
Collaborator

@tacruc tacruc commented Apr 23, 2019

Addressing toDo in #45

Signed-off-by: Arne Hamann <kontakt+github@arne.email>
Signed-off-by: Arne Hamann <kontakt+github@arne.email>
Signed-off-by: Arne Hamann <kontakt+github@arne.email>
Signed-off-by: Arne Hamann <kontakt+github@arne.email>
Signed-off-by: Arne Hamann <kontakt+github@arne.email>
Signed-off-by: Arne Hamann <kontakt+github@arne.email>
@tacruc tacruc changed the title [WIP] Non localized pictures devices Non localized pictures devices Apr 23, 2019
@tacruc tacruc requested a review from julien-nc April 23, 2019 17:48
Signed-off-by: Arne Hamann <kontakt+github@arne.email>
@tacruc
Copy link
Collaborator Author

tacruc commented Apr 24, 2019

For the lazy loading ICache is used. There is the issue that the cached entry is not deleted nextcloud/server#15208.

@eneiluj Should we programm a workaround and safe the timestamp manually in the cache and programm a deletion, or should we wait unit the issue is fixed on the server side?

@tacruc tacruc mentioned this pull request Apr 24, 2019
7 tasks
@julien-nc
Copy link
Member

Yay, seems nice!

It would be great if you could explain what you did in a few lines. A list of things you did technically, why you did it and what effect/benefit it has.

Should we programm a workaround and safe the timestamp manually...

I don't know since I didn't really get what the issue is. Even if the cache is not deleted, it probably does not generate a huge amount of information. I think we can implement Maps like if caching was working as expected.

What do you mean by "save the timestamp manually"? Do you mean without caching?

A first remark: callForImages was getting all nonLocphotos information in one request before, why did you make it do a request per photo? I'm sure there is a reason but I can't find out which 😄.

My point of view about getting a lot of elements with ajax requests is: by experience it's much faster and efficient (client CPU load) to get them all in one request if you want them all anyway. It's faster because making a HTTP request costs extra time which is probably bigger than the time needed to process the result. So if you repeat it hundreds of times, the time proportion taken by HTTP requests is huge. It's more efficient because you can factorize the processing of results.

I'm sorry if I say potentially obvious things or if I didn't exactly get the reasons of your choices 😄. I just want to be clear and explicit 🤓.

Ow and a very small remark: You fixed $this->timeordedPointSets by naming it $this->timeorderedPointSets but you didn't fix the loop variable name : foreach ($this->timeorderedPointSets as $timeordedPointSet) {

@tacruc
Copy link
Collaborator Author

tacruc commented Apr 24, 2019

Talking more messurements shows me that I was overshooting the goal.

Ow and a very small remark: You fixed $this->timeordedPointSets by naming it $this->timeorderedPointSets but you didn't fix the loop variable name : foreach ($this->timeorderedPointSets as $timeordedPointSet) {

I will fix that

@julien-nc
Copy link
Member

Ok I think I got it now. Neat!

Without explanation and without reading the code in detail it was hard for me to understand.

So from what I understand: Now, when Photos layer is enabled, js controller first gets the ids of all non-localized photos and then does one request per photo to ask server side controller to guess the coordinates and provide the photo entry. Each successful guess is cached on the server side.

This PR is not about pre-guessing with a cron-job or files/contacts/anything hooks.

Am I right?

So I agree with you about making multiple requests because the guessing is done at this moment. I thought you were just getting already guessed things from the DB.

On the other hand now the reading of the gpx files and devices files and the preprocessing is done on pageload so in advanced.

Is it really done on page load or just when Photos layer is getting enabled?

We need to be very explicit and descriptive when discussing those kind of features otherwise I say stupid things. 😄

@julien-nc
Copy link
Member

Btw,

I was misunderstanding the ICache documentation

Does it mean there is no bug in NC caching system?

@tacruc
Copy link
Collaborator Author

tacruc commented Apr 24, 2019

Yes the ttl is just the minimum time to life you should not expected it to be deleted after it

@tacruc
Copy link
Collaborator Author

tacruc commented Apr 24, 2019

The main Idea was not to wait for 10000 pictures that the location is computed, but to load them incrementally. I think loading every picture was too much, so I will change it to load batches of 20 pictures or something.

Signed-off-by: Arne Hamann <kontakt+github@arne.email>
Signed-off-by: Arne Hamann <kontakt+github@arne.email>
@tacruc
Copy link
Collaborator Author

tacruc commented Apr 24, 2019

Now it is doing what it is supposed to do. Reducing the loading time of pictures without geoinformation.
On pageload a list of all pictures is loaded. With this request the gpx files are loaded in the backend. Then it is fast to load batches of 250 Photos.
Reduces the loading time on my machine with 409 pictures from 4s to 0.9 seconds.

@tacruc
Copy link
Collaborator Author

tacruc commented Apr 25, 2019

I will close this

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.

None yet

3 participants