<HTML>
<HEAD>
<TITLE>Re: [vtk-developers] vtkMath inline choices...</TITLE>
</HEAD>
<BODY>
<FONT FACE="Calibri, Verdana, Helvetica, Arial"><SPAN STYLE='font-size:11pt'>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<BR>
<BR>
</SPAN></FONT><BLOCKQUOTE><FONT FACE="Calibri, Verdana, Helvetica, Arial"><SPAN STYLE='font-size:11pt'><a href="http://review.source.kitware.com/760">http://review.source.kitware.com/760</a><BR>
</SPAN></FONT></BLOCKQUOTE><FONT FACE="Calibri, Verdana, Helvetica, Arial"><SPAN STYLE='font-size:11pt'><BR>
Could we get a volunteer from Kitware to take a look at the changes?<BR>
<BR>
-Ken<BR>
<BR>
<BR>
On 1/21/11 2:28 PM, "Sean McBride" <<a href="sean@rogue-research.com">sean@rogue-research.com</a>> wrote:<BR>
<BR>
</SPAN></FONT><BLOCKQUOTE><FONT FACE="Calibri, Verdana, Helvetica, Arial"><SPAN STYLE='font-size:11pt'>Kenneth,<BR>
<BR>
I know it's been months, but I've finally learned enough git to pursue<BR>
this....<BR>
<BR>
<BR>
On Wed, 15 Sep 2010 09:24:14 -0600, Moreland, Kenneth said:<BR>
<BR>
>I can speak to why IsNan is not inlined. The IsNan (and similar non-<BR>
>finite tests) rely on some non-portable functionality. Depending on<BR>
>whether the isnan function is provided, IEEE-754 numbers are used, or if<BR>
>operations on NaNs are trapped, different implementations are provided.<BR>
>Over time this can get even more complicated as we encounter computers<BR>
>and compilers that treat NaNs differently. Quite simply, I did not want<BR>
>to expose all of this complexity in the header file.<BR>
<BR>
vtkMath::IsNan() is currently implemented like so:<BR>
<BR>
int vtkMath::IsNan(double x)<BR>
{<BR>
#ifdef VTK_HAS_ISNAN<BR>
return (isnan(x) ? 1 : 0);<BR>
#else<BR>
return !((x <= 0.0) || (x >= 0.0));<BR>
#endif<BR>
}<BR>
<BR>
I don't see any IEEE-754 assumptions/non-portable code there.... but<BR>
then I noticed isnan() itself is conditionally redefined. Evil. :)<BR>
<BR>
>If you plan to go through the trouble of moving that to the header file,<BR>
>you should do performance tests to make sure that there is a clear<BR>
>advantage to inlining the function. I've been surprised in the past to<BR>
>find an inline function did not really perform any better than a<BR>
>function call. The fact of the matter is that function calls are cheap<BR>
>operations nowadays.<BR>
<BR>
For me, in Release, vtkMath::IsNan() becomes:<BR>
<BR>
+0x0 pushq %rbp<BR>
+0x1 movq %rsp, %rbp<BR>
+0x4 xorl %eax, %eax<BR>
+0x6 ucomisd %xmm0, %xmm0<BR>
+0xa setpb %al<BR>
+0xd leave <BR>
+0xe ret <BR>
+0xf nop <BR>
<BR>
Profiling (by sampling) a slow spot in my app, I spend a full 5% of my<BR>
time in it (it's number #2). That's a lot of samples for a 7<BR>
instruction function. And of the samples that occur within it, 75% of<BR>
them are the initial 'pushq' or the 'ret'. I do believe inlining would<BR>
help here.<BR>
<BR>
What do you think of the attached patch? I inline IsNan only if<BR>
VTK_HAS_ISNAN is true, so the yucky stuff is still hidden in the .c. I<BR>
had to #include "vtkMathConfigure.h" in vtkMath.h, which would mean that<BR>
vtkMathConfigure.h would have to be 'installed' also. I don't know how<BR>
to do that.<BR>
<BR>
Cheers,<BR>
<BR>
--<BR>
____________________________________________________________<BR>
Sean McBride, B. Eng <a href="sean@rogue-research.com">sean@rogue-research.com</a><BR>
Rogue Research www.rogue-research.com<BR>
Mac Software Developer Montréal, Québec, Canada<BR>
<BR>
</SPAN></FONT></BLOCKQUOTE><FONT FACE="Calibri, Verdana, Helvetica, Arial"><SPAN STYLE='font-size:11pt'><BR>
</SPAN></FONT><FONT SIZE="2"><FONT FACE="Consolas, Courier New, Courier"><SPAN STYLE='font-size:10pt'><BR>
**** Kenneth Moreland<BR>
*** Sandia National Laboratories<BR>
*********** <BR>
*** *** *** email: <a href="kmorel@sandia.gov">kmorel@sandia.gov</a><BR>
** *** ** phone: (505) 844-8919<BR>
*** web: <a href="http://www.cs.unm.edu/~kmorel">http://www.cs.unm.edu/~kmorel</a><BR>
</SPAN></FONT></FONT><FONT FACE="Calibri, Verdana, Helvetica, Arial"><SPAN STYLE='font-size:11pt'><BR>
</SPAN></FONT>
</BODY>
</HTML>