[PATCH v3] tools: copyfile: use 64k instead of 512 buffer

[Dragan Simic]

Hello Ahelenia,

Just checking, have you forgotten about this patch?


On 2024-03-21 21:50, Dragan Simic wrote:
> On 2024-03-21 21:37, Ahelenia Ziemiańska wrote:
>> This is a trivial but significant optimization:
>> mkimage took >200ms (and 49489 writes (of which 49456 512)),
>> now it takes  110ms (and   419 writes (of which   386 64k)).
>> 
>> sendfile is much more appropriate for this and is done in one syscall,
>> but doesn't bring any significant speedups over 64k r/w
>> at the 13M size ranges, so there's no need to introduce
>> 	#if __linux__
>> 	while((size = sendfile(fd_dst, fd_src, NULL, 128 * 1024 * 1024)) > 0)
>> 		;
>> 	if(size != -1) {
>> 		ret = 0;
>> 		goto out;
>> 	}
>> 	#endif
>> 
>> Also extract the buffer size to a constant.
>> 
>> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli at nabijaczleweli.xyz>
>> Reviewed-by: Dragan Simic <dsimic at manjaro.org>
> 
> Please, remove my Reviewed-by tag from this version of the patch,
> because it no longer applies.
> 
>> ---
>> On Thu, Mar 21, 2024 at 08:49:52PM +0100, Dragan Simic wrote:
>>> On 2024-03-21 19:29, Ahelenia Ziemiańska wrote:
>>> > This is a trivial but significant optimisation:
>>> s/optimisation/optimization/
>> This seems to run counter to precedent of not doing americans'
>> imperialism for them for free
>> (I see -ise/-ize in free variation even in my 100-deep checkout).
> 
> Well, that's politics, which I have no interest about.
> 
>>> > +#define copyfile_bufsize (64 * 1024)
>>> This is not the right place for such a preprocessor directive.
>>> Instead, it should be placed at the start of the file.
>>> 
>>> Also, it should use all uppercase letters.
>> FTR, neither of these points seem to be universal;
>> I modelled this after tools/mtk_image.c (also cf. 
>> scripts/kconfig/expr.c),
>> but there are at least 25 other function-local macros
>> (as in git grep -B1 '^#define' | grep -c -A1 -e '-{' returns 27).
> 
> AFAICT, scripts/kconfig/expr.c could use come cleanups.
> 
>> Also kinda weird to explicitly request a macro
>> only to review it back to a constant, but whatever.
> 
> Well, it isn't weird.  See, the instances of a preprocessor #define
> directive are replaced with its definition before the source code
> reaches the compiler, making it, in this case, a bit easier for the
> compiler to do its job.
> 
> The way you wrote it below, the compiler will almost surely optimize
> out the constant variable and replace it with the literal number in
> the generated object code, but that puts unnecessary burden on the
> compiler, which can be avoided by feeding it with the literal number,
> using a preprocessor directive.
> 
>>  tools/fit_common.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/fit_common.c b/tools/fit_common.c
>> index 2d417d47..37066203 100644
>> --- a/tools/fit_common.c
>> +++ b/tools/fit_common.c
>> @@ -129,6 +129,7 @@ int copyfile(const char *src, const char *dst)
>>  {
>>  	int fd_src = -1, fd_dst = -1;
>>  	void *buf = NULL;
>> +	const size_t bufsize = 64 * 1024;
>>  	ssize_t size;
>>  	size_t count;
>>  	int ret = -1;
>> @@ -145,14 +146,14 @@ int copyfile(const char *src, const char *dst)
>>  		goto out;
>>  	}
>> 
>> -	buf = calloc(1, 512);
>> +	buf = calloc(1, bufsize);
>>  	if (!buf) {
>>  		printf("Can't allocate buffer to copy file\n");
>>  		goto out;
>>  	}
>> 
>>  	while (1) {
>> -		size = read(fd_src, buf, 512);
>> +		size = read(fd_src, buf, bufsize);
>>  		if (size < 0) {
>>  			printf("Can't read file %s\n", src);
>>  			goto out;