[vtk-developers] vtkMath inline choices...

[Moreland, Kenneth]

The patch looks OK to me, but I defer final judgment to those at Kitware that have to support it.  To this end, I have posted the commit to gerrit for review.  The URL is

http://review.source.kitware.com/760

Could we get a volunteer from Kitware to take a look at the changes?

-Ken


On 1/21/11 2:28 PM, "Sean McBride" <sean at rogue-research.com> wrote:

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



   ****      Kenneth Moreland
    ***      Sandia National Laboratories
***********
*** *** ***  email: kmorel at sandia.gov
**  ***  **  phone: (505) 844-8919
    ***      web:   http://www.cs.unm.edu/~kmorel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20110124/5395a7ac/attachment.html>