split fix android memory kill in split

[cre4ture]

split allocates quite some memory when reading from "files" with no real end like /dev/zero.
On x86 its ~ 1GB and on x86_x64 its 2GB. This was cause frequent stability issues on android CI.
This change limits the initially read memory heavily to the blksize of the filesystem or 500MB at max if no blksize is specified. This is aligned with a solution used in head

[github-actions]

GNU testsuite comparison:

GNU test failed: tests/split/b-chunk. tests/split/b-chunk is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/split/l-chunk. tests/split/l-chunk is passing on 'main'. Maybe you have to rebase?
GNU test error: tests/split/line-bytes. tests/split/line-bytes is passing on 'main'. Maybe you have to rebase?

[tertsdiepraam]

I've only given this a cursory glance, but we do probably need to at least keep that ---io-blksize argument around. Why did you remove it?

[cre4ture]

I've only given this a cursory glance, but we do probably need to at least keep that ---io-blksize argument around. Why did you remove it?

I do not see the purpose. Its a hidden (undocumented?) parameter. Who will gona use it? Does GNU have it?
Of course it doesn't hurt much. But I thought automatic blksize is better than manual.

[tertsdiepraam]

Yeah GNU has it, and that's why we have it too. It's also used us some of their tests, so if we want to pass those we need it.

[cre4ture]

ah, ok, I see. I'll add it again.

[cre4ture]

How do we know what the purpose of the undocumented argument is about?
It turns out that a empty implementation of that argument makes the tests green.

[tertsdiepraam]

Yeah it's mostly for testing I believe, but it'd be nice to make it useful if we have it :)

[tertsdiepraam]

I have a couple of questions/suggestions about this.

• It looks like we're only using the result as u64, is that correct? Maybe we could just make it use u64 instead of usize. That'll allow us to get rid of the conversions and simplify the code a bit.
• I don't think the stdin check is really correct, because we might be piping in a file with a well-defined size. So, I think the check should not just be checking for -.
• We should think about the naming of the functions and document all the public functions. Maybe sane_blksize? I'd love to get rid of the distinction between the path and metadata versions too, but if that's not possible this is alright.

[cre4ture]

Thanks for reviewing and feedback :-)
Response below:

I have a couple of questions/suggestions about this.

• It looks like we're only using the result as u64, is that correct? Maybe we could just make it use u64 instead of usize. That'll allow us to get rid of the conversions and simplify the code a bit.

I was switching to usize because the value we return should always fit into a usize type.
I'm not yet fully familiar with the rust strategy here. I've seen that in many places there is just the u64 used even though a usize would probably be more efficient. Especially when we consider 32bit systems as well.
So I can change it back to u64 as in the original implementation in head.
But if you could point me to a document which tells more about the general strategy, I would be glad.

• I don't think the stdin check is really correct, because we might be piping in a file with a well-defined size. So, I think the check should not just be checking for -.

You are right. Thinking about it a bit more I came to the conclution that I can just remove that code as the function will anyway return the DEFAULT when the call to metadata() fails. And the call to metadata() should fail in case of dash input "-".

• We should think about the naming of the functions and document all the public functions. Maybe sane_blksize? I'd love to get rid of the distinction between the path and metadata versions too, but if that's not possible this is alright.

When removing the special handling for "-", then the path variant will be purely for convenience and can be removed. Is this was you mean?

[tertsdiepraam]

Great!

I think for code paths like these, u64 vs usize will barely make a difference in performance. The more important thing you mention is that it should indeed fit into usize, so a usize might make sense. However, generally my approach is to avoid conversions as much as possible. If an API from std returns u64, for example, they probably have good reason for that.

The solution for - works, I think, but I do think we need to put a comment on it to say that we can do even better. The thing is that we should actually try to get the metadata of stdin on -. This is a larger issue in the utils we need to think about though. I'd love to improve the handling of - across the utils and make it more consistent. Ideas on that are very welcome :)

I think the path version is adding something useful in the end, so let's keep it.

[tertsdiepraam]

Looking good! Just a few more questions

[cakebaker]

Thanks!