Public/pics
2
2
Fork 2
Code Issues 6 Pull Requests 2 Releases Wiki Activity

Fixes album export by reducing memory usage of the scripts. #18

Closed
Roflin wants to merge 1 commits from fix-album-export into master
pull from: fix-album-export
merge into: Public:master
Conversation 1 Commits 1 Files Changed 2 +14 -17
2 changed files with 14 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 9d10d53d9f - Show all commits

2
app.php
View File

@ -27,6 +27,6 @@ set_error_handler('ErrorHandler::handleError');
ini_set("display_errors", DEBUG ? "On" : "Off");
// The real magic starts here!
ob_start();
ob_start(null, 32768);
minnozz commented 2020-02-26 22:17:37 +01:00
Review
Copy Link

I haven't looked at the rest of the framework, but you might want to consider disabling the output buffer before calling tar. This code could then check whether an output buffer is still active with ob_get_level.

I haven't looked at the rest of the framework, but you might want to consider disabling the output buffer before calling `tar`. This code could then check whether an output buffer is still active with [`ob_get_level`](https://www.php.net/manual/en/function.ob-get-level.php).
Roflin commented 2020-02-26 22:20:13 +01:00
Review
Copy Link

@aaron what are your thoughts on this? I'm not familiar enough to comment on this.

@aaron what are your thoughts on this? I'm not familiar enough to comment on this.
Aaron commented 2020-02-26 23:43:44 +01:00
Review
Copy Link

I agree with @minnozz; much better to disable it locally (i.e. in the download controller) with ob_end_flush. In general, output buffering is quite useful.

I agree with @minnozz; much better to disable it locally (i.e. in the download controller) with `ob_end_flush`. In general, output buffering is quite useful.
Dispatcher::dispatch();
ob_end_flush();

29
controllers/Download.php
View File

@ -53,20 +53,6 @@ class Download extends HTMLController
{
$files = array();
$album_ids = array_merge(array($album->id_tag), $this->getChildAlbumIds($album->id_tag));
foreach($album_ids as $album_id)
{
$iterator = AssetIterator::getByOptions(
array(
'id_tag' => $album_id
)
);
while($asset = $iterator->Next())
{
$files[] = join(DIRECTORY_SEPARATOR, array('.', $asset->getSubdir(), $asset->getFilename()));
minnozz commented 2020-02-26 22:12:47 +01:00
Review
Copy Link

Is this change still useful? It seems like an array with only paths ($files) and not the file contents should not get so large as to hit a memory limit.

Is this change still useful? It seems like an array with only paths (`$files`) and not the file contents should not get so large as to hit a memory limit.
Roflin commented 2020-02-26 22:18:48 +01:00
Review
Copy Link

In the old situation the export would fail on the first chunk leaving behind a .tar of ~0kb, This means that the list of files of an album of around 270 pictures already consumed a considerable amount of memory. So I would still like to keep this more "streaming" approach as a optimization.

In the old situation the export would fail on the first chunk leaving behind a .tar of ~0kb, This means that the list of files of an album of around 270 pictures already consumed a considerable amount of memory. So I would still like to keep this more "streaming" approach as a optimization.
minnozz commented 2020-02-26 22:31:17 +01:00
Review
Copy Link

I don't think that's possible, because if the memory limit was hit while building the array of file names, there would be no tar file at all, since tar is invoked after $files has been built.

While I don't think the streaming approach is wrong, this feels like a premature optimization (and certainly one that should not be necessary).

I don't think that's possible, because if the memory limit was hit while building the array of file names, there would be no tar file at all, since `tar` is invoked after `$files` has been built. While I don't think the streaming approach is wrong, this feels like a premature optimization (and certainly one that should not be necessary).
}
}
$descriptorspec = array(
minnozz commented 2020-02-26 22:15:11 +01:00
Review
Copy Link

The current code uses proc_open, but you might want to look into passthru.

The current code uses [`proc_open`](https://www.php.net/manual/en/function.proc-open.php), but you might want to look into [`passthru`](https://www.php.net/manual/en/function.passthru.php).
Roflin commented 2020-02-26 22:23:12 +01:00
Review
Copy Link

I'm currently using ther tar's -T functionality to read a list of files in a file seperated by a "\0" byte. and I skipping the file all together by using - as the argument to -T and feeding it the files on STDIN. After a quick look at the passthru i don't think such a thing is possible. and I think i will run out of "command line" space to execute the tar command with passthru.

I'm currently using ther tar's -T functionality to read a list of files in a file seperated by a "\0" byte. and I skipping the file all together by using - as the argument to -T and feeding it the files on STDIN. After a quick look at the passthru i don't think such a thing is possible. and I think i will run out of "command line" space to execute the tar command with passthru.
Aaron commented 2020-02-26 23:48:09 +01:00
Review
Copy Link

Agreed that passthru is probably the better solution, especially if you're not expecting to handle anything on STDOUT or STDERR.

I do not think the proposed solution to use STDIN to manually feed filenames will provide much in the way of performance benefit. If anything, I think tar can enumerate files more quickly than a PHP script.

Agreed that `passthru` is probably the better solution, especially if you're not expecting to handle anything on `STDOUT` or `STDERR`. I do not think the proposed solution to use `STDIN` to manually feed filenames will provide much in the way of performance benefit. If anything, I think `tar` can enumerate files more quickly than a PHP script.
0 => array('pipe', 'r'),
1 => array('pipe', 'w'),
@ -93,8 +79,19 @@ class Download extends HTMLController
header('Content-Type: application/octet-stream');
header('Content-Transfer-Encoding: binary');
foreach($files as $file)
fwrite($pipes[0], $file . "\0");
$album_ids = array_merge(array($album->id_tag), $this->getChildAlbumIds($album->id_tag));
foreach($album_ids as $album_id)
{
$iterator = AssetIterator::getByOptions(
array(
Aaron commented 2020-02-26 23:45:21 +01:00
Review
Copy Link

This is an aside, but I'd like to stick to new-style (bracketed) arrays, that is [] instead of array().

This is an aside, but I'd like to stick to new-style (bracketed) arrays, that is `[]` instead of `array()`.
'id_tag' => $album_id
)
);
while($asset = $iterator->Next())
{
fwrite($pipes[0], join(DIRECTORY_SEPARATOR, array('.', $asset->getSubdir(), $asset->getFilename())) . "\0");
}
}
fclose($pipes[0]);