Re: Perf patches, libmpg123 patches, and a new release of LAME?

Quoting Gabriel Bouvigne <[email protected]> (from Wed, 06 May  
2020 09:54:01 +0000):

> On 2020-05-05 14:17, Alexander Leidinger via Lame-dev wrote:
>
>> 1. perf patches:
>> Do we have someone with some SSE knowledge here who could review
>> https://tmkk.undo.jp/lame/lame-3.100-sse-20171014.diff? I had a look
>> at it, the SSE part is out of my comfort zone. I noticed that the IEEE
>> Hack from Takehiro is disabled in this patch, and if you look at the
>> graph in https://tmkk.undo.jp/lame/index_e.html it seems that with
>> current CPUs it is not beneficial anymore. Another thing I noticed is
>> that an assert is commented out. No idea why.
>>
>> So if someone could review if what is there is a sane approach (also
>> in terms of difference in the FP precission!) we could maybe integrate
>> this speed improvement.
>>
>> There is also a patch for a faster CRC routine:
>>   https://hydrogenaud.io/index.php?topic=115900.0
>> Anyone up to have a look at this?
>
>
> Disclaimer:
> please note that I am voicing my opinion, but considering that I  
> haven't contributed to Lame for ages and that it's unlikely that I  
> will in the short future, please take my opinion with a pinch of salt.
>
> Hello everyone,
>
> Regarding the perf patches:
>
> - Takehiro IEEE754 hack
> It indeeds slows things down on most of the modern processors (at  
> least on current AMD processors, and all of the i3/i5/i7 line from  
> Intel), and it has been this way for at least 10 years. I am however  
> not sure about its performance impact on low-grade Intel processors  
> (the current "Pentium" and Atom line, which are significantly  
> different from the i3/5/7 line).
> I see two options there:
> a) keep the hack code, but conditionnaly enable it based on target processor.
> b) totally disable the hack.
>
> Unless we have the ability to test the speed benefits on Pentiums  
> and Atoms beforehand, a good option might be to:
> *for next version, keep the code, but globally disable the hack
> *based on eventual field reports, decide if we should remove the  
> code for the next next version

That is what I had in mind myself. I'm a little bit indifferent if I  
should provide a configure option to enable it or not.

> - Changes from https://tmkk.undo.jp/lame/index_e.html
> The big issue is that those are using inline assembly, which are not  
> very portable. Intrisics would of course be more portable. That  
> being said, if I remember correctly, we already have the issue with  
> the nasm files wich are only used in 32bits mode (thus not very  
> portable)

Yes, and the patch comes with the ASM in an ifdef, so no change for  
those systems without the possibility to compile the ASM, and a speed  
benefit for those with the possibility.

Did you had a look at the code in question, does it look OK and do you  
worry about differences in FP precission?

> - Faster CRC patch
> This one *looks* ok, even though the speed gain is relatively small.

That was my first thought too, but on the other hand, if you use LAME  
to encode multiple independend streams at the same time and those  
streams are not only a few minutes of recording, it may matter to  
someone.

Bye,
Alexander.

-- 
http://www.Leidinger.net [email protected]: PGP 0x8F31830F9F2772BF
http://www.FreeBSD.org    [email protected]  : PGP 0x8F31830F9F2772BF