Public/pics
2
2 Fork 3
Code Issues 5 Pull Requests 1 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 4 Commits 1 Files Changed 2
+14 -17
Roflin commented 2020-02-26 21:47:44 +01:00
Owner
Copy link
Copy source

Chunks the output buffer to 4kB chunks. This means the Download.php
script will no longer run out of memory when trying to stream a
tar file the output buffer.

Chunks the output buffer to 4kB chunks. This means the Download.php script will no longer run out of memory when trying to stream a tar file the output buffer.
controllers/Download.php
@@ -63,4 +56,0 @@
);
while($asset = $iterator->Next())
{
$files[] = join(DIRECTORY_SEPARATOR, array('.', $asset->getSubdir(), $asset->getFilename()));
minnozz commented 2020-02-26 22:12:47 +01:00
Member
Copy link
Copy source

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
Author
Owner
Copy link
Copy source

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
Member
Copy link
Copy source

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).
controllers/Download.php
@@ -67,4 +56,1 @@
}
}
$descriptorspec = array(
minnozz commented 2020-02-26 22:15:11 +01:00
Member
Copy link
Copy source

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
Author
Owner
Copy link
Copy source

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
Member
Copy link
Copy source

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.
app.php
@@ -28,3 +28,3 @@
// The real magic starts here!
ob_start();
ob_start(null, 32768);
minnozz commented 2020-02-26 22:17:37 +01:00
Member
Copy link
Copy source

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
Author
Owner
Copy link
Copy source

@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
Member
Copy link
Copy source

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.
controllers/Download.php
@@ -98,0 +83,4 @@
foreach($album_ids as $album_id)
{
$iterator = AssetIterator::getByOptions(
array(
Aaron commented 2020-02-26 23:45:21 +01:00
Member
Copy link
Copy source

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()`.
Roflin commented 2020-03-11 17:48:33 +01:00
Author
Owner
Copy link
Copy source

This is being superseded by PR !19

This is being superseded by PR !19
Roflin closed this pull request 2020-03-11 17:48:35 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
minnozz
Aaron
Labels
No items
No labels
Milestone
No items
No Milestone
Assignees
Clear assignees
Aaron (Aaron van Geffen) minnozz (Bart Schuurmans) mrngm Roflin yorick
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Public/pics#18