[vtk-developers] vtkMath inline choices...

[Sean McBride]

Kenneth,

I know it's been months, but I've finally learned enough git to pursue
this....


On Wed, 15 Sep 2010 09:24:14 -0600, Moreland, Kenneth said:

>I can speak to why IsNan is not inlined.  The IsNan (and similar non-
>finite tests) rely on some non-portable functionality.  Depending on
>whether the isnan function is provided, IEEE-754 numbers are used, or if
>operations on NaNs are trapped, different implementations are provided. 
>Over time this can get even more complicated as we encounter computers
>and compilers that treat NaNs differently.  Quite simply, I did not want
>to expose all of this complexity in the header file.

vtkMath::IsNan() is currently implemented like so:

int vtkMath::IsNan(double x)
{
#ifdef VTK_HAS_ISNAN
  return (isnan(x) ? 1 : 0);
#else
  return !((x <= 0.0) || (x >= 0.0));
#endif
}

I don't see any IEEE-754 assumptions/non-portable code there.... but
then I noticed isnan() itself is conditionally redefined.  Evil. :)

>If you plan to go through the trouble of moving that to the header file,
>you should do performance tests to make sure that there is a clear
>advantage to inlining the function.  I've been surprised in the past to
>find an inline function did not really perform any better than a
>function call.  The fact of the matter is that function calls are cheap
>operations nowadays.

For me, in Release, vtkMath::IsNan() becomes:

+0x0	pushq	%rbp
+0x1	movq	%rsp, %rbp
+0x4	xorl	%eax, %eax
+0x6	ucomisd	%xmm0, %xmm0
+0xa	setpb	%al
+0xd	leave	
+0xe	ret	
+0xf	nop	

Profiling (by sampling) a slow spot in my app, I spend a full 5% of my
time in it (it's number #2).  That's a lot of samples for a 7
instruction function.  And of the samples that occur within it, 75% of
them are the initial 'pushq' or the 'ret'.  I do believe inlining would
help here.

What do you think of the attached patch?  I inline IsNan only if
VTK_HAS_ISNAN is true, so the yucky stuff is still hidden in the .c.  I
had to #include "vtkMathConfigure.h" in vtkMath.h, which would mean that
vtkMathConfigure.h would have to be 'installed' also.  I don't know how
to do that.

Cheers,

-- 
____________________________________________________________
Sean McBride, B. Eng                 sean at rogue-research.com
Rogue Research                        www.rogue-research.com 
Mac Software Developer              Montréal, Québec, Canada
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ENH-made-vtkMath-IsInf-and-vtkMath-IsNan-inlined-som.patch
Type: application/octet-stream
Size: 5373 bytes
Desc: not available
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20110121/1850774c/attachment-0001.obj>