read larger chunks from sftp in stream wrapper

[icewind1991]

This speeds up reading from the stream wrapper about 5x in my local testing, increasing the chunk size further doesn't seem to make a difference

[bantu]

@icewind1991 Hi Robin. I've not been active in this project for quite some time, so please bear with me.

My understanding is that, essentially, you're introducing a buffer between (external) SFTP requests and (internal) PHP stream requests, correct? That seems to make sense to me.

[bantu]

Can we make the 81920 a parameter? I think there is a mechanism for that on stream construction.

[terrafrost]

I think what bantu is talking about is this:

<?php
require __DIR__ . '/vendor/autoload.php';

use phpseclib3\Net\SFTP\Stream;
use phpseclib3\Net\SFTP;

Stream::register();

$sftp = new SFTP('website.com');
$sftp->login('username', 'password');

$context = [
    'sftp' => ['sftp' => $sftp]
];
$context = stream_context_create($context);

$fp = fopen('sftp://dummy.com/home/vagrant/1mb', 'r', false, $context);
echo fread($fp, 10);

Other options are discussed at https://www.php.net/manual/en/wrappers.ssh2.php

[bantu]

Yes, "stream context" is what I was referring to.

[terrafrost]

For the most common use case this does indeed appear to speed things up a good amount. A much less common use case, however, would be slowed down by this. eg. randomly seeking / reading 1kb chunks of a file.

That said, I noticed you targeted the master branch with this PR. Maybe 3.0 might not be a bad idea? I say that because long term I do want to do something like #655 (comment) , which could very well eliminate any speedups your PR provides.

[terrafrost]

This is nitpicky but... you could do $result = \phpseclib3\Common\Functions\Strings::shift($this->buffer, $count) instead. Also, I'd do $this->buffer.= $chunk instead of $this->buffer .= $chunk.

[bantu]

What if

• multiple chunks are loaded into the buffer
• the first one is read from the stream (i.e. buffer), modified, and written back to the underlying file of the stream at chunk 2

Does the stream still behave in accordance to its contracts? Pre-patch it would probably return the newly written data at chunk 2 and post-patch it would return stale data from the buffer.

[terrafrost]

@bantu - good points. Maybe it'd just be easier if I implemented the ideas discussed in #655 (comment) . I could do that in the 1.0 / 2.0 / 3.0 / master branches. That's not a BC breaking change, after all.

[bantu]

@terrafrost Maybe call the parameter something like "readahead length" and make it optional. This way Robin can use it for his use case. If the parameter is not supplied, it should behave as before.

@icewind1991 What do you think?

[icewind1991]

made the readahead length configurable and added cache invalidation logic to prevent reading stale chunks.

[bantu]

added cache invalidation logic to prevent reading stale chunks

In the buffering problem I described in #1452 (comment), I was (implicitly) considering two independent instances accessing the file. Your additional changes do not work in this case. Hence my suggestion for not changing the behavior in case the read ahead buffer length is not supplied.

[amayer5125]

Should this say @access private?

[terrafrost]

Here's my idea for an alternate way to deal with this:

https://github.com/terrafrost/phpseclib/tree/1.0-sftp-file-handling

I'm thinking that this - and making nlist and rawlist return an ArrayObject instead of an actual array (see #1418) - could create new versions of phpseclib - 1.1.0, 2.1.0 and 3.1.0.

What I've done can be used as a starting point. If no one else wants to take this task on I can do so in the fullness of time.