Re: [PATCH v5 07/10] perf record: implement -z,--compression_level=n option and compression

[Alexey Budankov]

On 05.03.2019 15:25, Jiri Olsa wrote:
> On Fri, Mar 01, 2019 at 06:58:32PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>  
>>  	/*
>>  	 * Increment md->refcount to guard md->data[idx] buffer
>> @@ -350,7 +357,7 @@ int perf_mmap__aio_push(struct perf_mmap *md, void *to, int idx,
>>  	md->prev = head;
>>  	perf_mmap__consume(md);
>>  
>> -	rc = push(to, &md->aio.cblocks[idx], md->aio.data[idx], size0 + size, *off);
>> +	rc = push(to, md->aio.data[idx], size0 + size, *off, &md->aio.cblocks[idx]);
>>  	if (!rc) {
>>  		*off += size0 + size;
>>  	} else {
>> @@ -556,13 +563,15 @@ int perf_mmap__read_init(struct perf_mmap *map)
>>  }
>>  
>>  int perf_mmap__push(struct perf_mmap *md, void *to,
>> -		    int push(struct perf_mmap *map, void *to, void *buf, size_t size))
>> +		    int push(struct perf_mmap *map, void *to, void *buf, size_t size),
>> +		    perf_mmap__compress_fn_t compress, void *comp_data)
>>  {
>>  	u64 head = perf_mmap__read_head(md);
>>  	unsigned char *data = md->base + page_size;
>>  	unsigned long size;
>>  	void *buf;
>>  	int rc = 0;
>> +	size_t mmap_len = perf_mmap__mmap_len(md);
>>  
>>  	rc = perf_mmap__read_init(md);
>>  	if (rc < 0)
>> @@ -574,7 +583,10 @@ int perf_mmap__push(struct perf_mmap *md, void *to,
>>  		buf = &data[md->start & md->mask];
>>  		size = md->mask + 1 - (md->start & md->mask);
>>  		md->start += size;
>> -
>> +		if (compress) {
>> +			size = compress(comp_data, md->data, mmap_len, buf, size);
>> +			buf = md->data;
>> +		}
>>  		if (push(md, to, buf, size) < 0) {
>>  			rc = -1;
>>  			goto out;
> 
> when we discussed the compress callback should be another layer
> in perf_mmap__push I was thinking more of the layered/fifo design,
> like:
> 
> normaly we call:
> 
> 	perf_mmap__push(... push = record__pushfn ...)
> 		-> reads mmap data and calls push(data), which translates as:
> 
> 		record__pushfn(data);
> 			- which stores the data
> 
> 
> for compressed it'd be:
> 
> 	perf_mmap__push(... push = compressed_push ...)
> 
> 		-> reads mmap data and calls push(data), which translates as:
> 
> 		compressed_push(data)
> 			-> reads data, compresses them and calls, next push callback in line:
> 
> 			record__pushfn(data)
> 				- which stores the data
> 
> 
> there'd need to be the logic for compressed_push to
> remember the 'next push' function

That is suboptimal for AIO. Also compression is an independent operation that 
could be applied on any of push stages you mean.

> 
> but I think this was the original idea behind the
> perf_mmap__push -> it gets the data and pushes them for
> the next processing.. it should stay as simple as that

Agree on keeping simplicity and, at the moment, there is no any push to the next 
processing in the code so provided implementation fits as for serial as for AIO
at the same time sticking to simplicity as much as possibly. If you see something 
that would fit better please speak up and share.

~Alexey

> 
> jirka
>