Re: [PATCH dwarves v2 07/10] btf_encoder: introduce btf_encoding_context

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Dec 17, 2024 at 04:40:40PM -0800, Eduard Zingerman wrote:
> On Tue, 2024-12-17 at 16:03 -0800, Andrii Nakryiko wrote:
> 
> [...]
> 
> > I agree with Ihor. I think he invested a lot of time into these
> > improvements, and asking him to re-do the series just to shuffle a few
> > patches around is just an unnecessary overhead (which also delays the
> > ultimate outcome: faster BTF generation with pahole).
> 
> Patches #1-4 are good refactorings.
> Patches #5-7 introduce a global state and complication where this
> could be avoided. (These were necessary before Ihor figured out the
> trick with patch #10).
> E.g. 'elf_functions_list':
> - there is no reason for it to be global, it could be a part of the
>   encoder, as it is now;
> - there is no reason for it to be a list, encoding works with a single
>   function table and keeping it as a list just confuses reader.

agree, with just single encoder object I don't see a reason for the
btf_encoding_context, keeping functions in encoder will be simpler

thanks,
jirka

> 
> Same for btf_encoder_list_lock / btf_encoder_list.
> 
> > And as Ihor mentioned, we might improve upon this series by
> > parallelizing encoders to gain some further improvements, so I think
> > all the internal refactoring and preparations are setting up a good
> > base for further work.
> 
> Not really, the main insight found by Ihor is that parallel BTF
> encoding doesn't make much sense: constructing BTF is as cheap as
> copying it. I don't see much room for improvement here.
> 
> [...]
> 
>