[MPlayer-dev-eng] [PATCH] revert CPU speed detection

[D Richard Felker III]

On Thu, Oct 07, 2004 at 11:31:47AM +0200, Diego Biurrun wrote:
> D Richard Felker III writes:
> > On Thu, Oct 07, 2004 at 04:34:50AM +0200, Diego Biurrun wrote:
> > > 
> > > I've had a look at a way to make the CPU speed detection run only in
> > > verbose mode but then reconsidered.  Speed detection has never worked
> > > reliably and slows down startup noticeably.  Thus I propose the
> > > following patch that rips out CPU speed detection by reverting the
> > > commit from Atmos that added it:
> > > 
> > > http://mplayerhq.hu/pipermail/mplayer-cvslog/2003-September/016619.html
> > > 
> > > What do you think?
> > 
> > i'm against applying this patch as-is. it's just a reversed version of
> > the original patches. granted those seem to have had some cosmetics,
> > but reverting that now is just more cosmetics.
> 
> Not reverting the cosmetics defeats the purpose of forbidding
> cosmetics in the first place.  No cosmetics lead to cleaner CVS diffs,
> if you keep the cosmetics now, the diffs remain unreadable, so what
> are we supposed to gain by not removing the cosmetics?

right now we have one unreadable diff. if we commit your patch we have
two unreadable diffs. 2>1 :)

> > also there's some useful stuff, like the hasTSC flag, which
> > shouldn't be reverted, and it's especially bad to change this back
> > in cpudetect.h which is otherwise untouched by this patch.
> 
> It's used nowhere else according to grep, neither are the other
> cpuspeed detection functions.

right, but it perhaps should be.

> I'm not against cherry picking the good parts out of the speed
> detection code.  IMO this should be done in a separate patch, though.
> That patch will be clean and easy to verify just as this one.  This
> would also separate each logical step into one commit as it should be
> done.  One cleanly backs out speed detection, one small patch adds TSC
> detection.

i'm VERY MUCH against committing one patch that removes hasTSC from
cpudetect.h and then another that adds it back, since this is the only
change made to cpudetect.h! it's just cvs pollution.

> > i vote for just removing the speed detection code itself, and none of
> > the other reversals. that makes the new patch much cleaner and easy to
> > tell what it does (and clear that there are no bugs).
> 
> The patch is clean and easy to verify.  How can I make such bold
> claims?  I did not create the patch myself, CVS did it for me.
> 
> Just look at the "cvs diff" headers:
> 
> The first hunk was created by
> 
>   cvs diff -r 1.31 -r 1.29 cpudetect.c

did you even check what patch 1.31 is? i expect 1.29->1.30 was the
speed crap and 1.30->1.31 was something actually useful. reversing
them together is nonsense!

rich