rework to support multiple concurrent instances #166
Conversation
Signed-off-by: nachoparker <nacho@ownyourbits.com>
|
@nachoparker Thank you so much for your PoC and your branch merge request. Considering your results, your tweaks would improve vastly the processing times in the Preview Generator app. I hope it gets merged anytime soon :) |
|
@nachoparker Well here an implementation has already been coded, many thanks for this! |
| // Respect the '.nomedia' file. If present don't traverse the folder | ||
| if ($folder->nodeExists('.nomedia')) { | ||
| $this->output->writeln('Skipping folder ' . $folder->getPath()); | ||
| return; | ||
| } | ||
|
|
||
| // random sleep between 0 and 50ms to avoid collision between 2 processes | ||
| usleep(rand(0,50000)); |
There was a problem hiding this comment.
Could we not get around this with a DB transaction?
There was a problem hiding this comment.
Indeed, you could get a lock by querying a row where file is node.id and locked is false, setting locked to true.
If this returns a result, you have the lock. If no lock, skip/wait.
There was a problem hiding this comment.
Hi,
I am not sure I understand what you both mean. Ideally we wouldn't have to do this if we could use some kind of pthreads/parallel approach but since ZST packages are not available in current Debian systems, or even in Sury, I am calling the occ command several times in parallel.
Indeed, you could get a lock by querying a row where file is node.id and locked is false, setting locked to true.
If this returns a result, you have the lock. If no lock, skip/wait.
Since we can't control different threads, but we have independent processes, then I added the lock column to the database so that two different instances don't start working on the same preview at the same time.
If you look at the code, that is exactly what I do, but because there is a chance that both instances read the lock value as unlocked at the same time before setting it as locked, I added an ugly random wait to minimize the chance of this happening.
This wouldn't be needed if we had multithread approach with barriers, waits and so on. (or just give each child thread a different group of previews to work on).
I hope it is clear now. This is not particularly pretty but given the limitations we have it has proven very much effective :)
There was a problem hiding this comment.
I am no PHP or SQL coder, but isn't there a way to combine a lock check + lock creation to eliminate the race condition?
E.g. in shell I do something like this:
set -C
if { > process.lock; } 2> /dev/null; then
start_process
else
skip_process
fi
set +C
set -Cadds the noclobber option, thus nothing can be written to existing files.- Assuming that two threads cannot write the same file at the same time, one write (empty redirect to lock file) must fail, which results in process is skipped.
- So basically checking for a lock and creating the lock is one and the same quick task. And the check practically occurs after the lock was created (checking exit code).
- Isn't there something similar possible for the preview generation processes, through PHP or SQL?
There was a problem hiding this comment.
The idea is to ran several instances simultaneously
for i in `seq 1 16`; do
occ_command &
done
wait
I want several processes in parallel, so the locking needs to be in the PHP section
There was a problem hiding this comment.
Don't need return code which checks for success (you are thinking shell scripting), we can just work with SQL to see if the column is locked or a new PID column matches, that's quite straightforward.
There's many ways to skin a cat. Deleting at the very end of the process would be an easy improvement but I don't want to keep going if this is going nowhere so I am awaiting feedback from the maintainer.
There was a problem hiding this comment.
I've been working on a python version of the preview generator with jpegtran for blazing fast image processing.
This snippet allows running multiple python processes.
for fileid, storage, name, path in image_filecache_query:
# check preview dir
try:
cursor.execute("INSERT INTO sg_preview (path, pid) VALUES ('{0}_{1}', {2})".format(storage, path, os.getpid()))
if not cursor:
continue
conn.commit()
except MySQLdb.Error:
continue
process_image(...)I'll let you know when it's ready to be published :)
There was a problem hiding this comment.
With Python as dependency IMO not a solution for the default app, but nice as alternative, if it's finally more efficient/faster and/or for systems where Python is available anyway.
There was a problem hiding this comment.
@stefaang sounds cool, great. I imagine it spawns threads in parallel or uses multithreading per image?
That would be a nice thing to have even though ideally this would be implemented in multithreading PHP in the long run.
But hey, that might never happen so kudos
There was a problem hiding this comment.
Don't need return code which checks for success (you are thinking shell scripting), we can just work with SQL to see if the column is locked or a new PID column matches, that's quite straightforward.
That would not solve the race condition. The problem is the time between checking the fields state/value and setting new state/value.
E.g. two parallel processes/threads by chance check the same field nearly the same time. One checks slightly earlier but it can take slightly too long for it to set the locked state/value, so the other process still finds the filed unlocked during this minimal but existing time frame => both process the same file.
So generally when doing a check and setting a value based on the checks result, this strictly inherits such a race condition. For this reason it would be great to skip the check and set the value directly. But this requires a set function that fails conditionally and this failure must be recognised afterwards to decide whether to go on processing or skipping it.
This matches as well what @stefaang suggested above schematically.
But yeah, I think you are right that before discussing such polishing, the general decision whether this is wanted or not and on which level the multi-threading should be done, has been made.
|
@rullzer The travis-ci build job fails because nextcloud requires atleast PHP 7.1, does this need to be fixed (aka PHP 7.0 removed from the pipeline)? |
|
@nachoparker just wondering if this is still alive - is there a todo you have, unfinished business? |
|
@jospoortvliet last time I checked we are waiting for feedback from the maintainer confirming we want this. Since this has not happened I am shipping NCP with my own fork of this app for the time being until that happens. |
Ok, so we're waiting for @rullzer - fair enough. |
@jospoortvliet @nachoparker It looks like @rullzer has abandoned this project? |
|
Since there was just a commit, obviously not 🙂: 03665bc |
Awesome. The commit was just a few hour after my post. It had been dormant for a while. Does not seem like this support for multiple concurrent instances is coming in v3 (yet) though? |
Hi, any progress? I couldn't apply @nachoparker's patches to current release :( |
|
Hi, kindly asking for progress :-) |
|
@stefaang or @nachoparker , is there any progress on this? |
|
Hey @nachoparker or any-one else, is there any status update on this after 2 years? We've gone through the pandemic and I feel like it has gone through faster than my preview images. |
See nachoparker#1
Signed-off-by: nachoparker nacho@ownyourbits.com