I agree with Hans that we should not delay the release. I also agree with Brad that we need to address this particular performance issue. We should make this a high priority for the next release.
Bill On Tue, Jul 3, 2012 at 10:35 AM, Johnson, Hans J <[email protected]>wrote: > Brad, > > My opinion is that this has been an issue for many years, and that the > cost benefit is not worth trying to squeeze this into this release cycle. > The results are (or at least should be) the same both before and after > this change. > > The backlog of new features and other performance improvements are > beginning to back up on the gerrit dashboard, so I would definitely vote > for cutting ITKv4.2 as soon as possible so that all other projects can > start moving forward again. > > My guess is that the main culprits of those failing tests are related to > following 15 lines of code. > > ============================== > johnsonhj@neuron$ git grep "\->GetInput()\->GetPixel" > Modules/Filtering/DistanceMap/include/itkSignedMaurerDistanceMapImageFilter.hxx: > if ( this->GetInput()->GetPixel(idx) != this->m_BackgroundValue ) > Modules/Filtering/ImageGrid/include/itkCyclicShiftImageFilter.hxx: > outIt.Set( static_cast< OutputImagePixelType >( > this->GetInput()->GetPixel( index ) ) ); > Modules/Filtering/MathematicalMorphology/include/itkGrayscaleConnectedClosingImageFilter.hxx: > seedValue = this->GetInput()->GetPixel(m_Seed); > Modules/Filtering/MathematicalMorphology/include/itkGrayscaleConnectedOpeningImageFilter.hxx: > seedValue = this->GetInput()->GetPixel(m_Seed); > Modules/Numerics/Statistics/include/itkScalarImageToRunLengthMatrixFilter.hxx: > pixelIntensity = this->GetInput()->GetPixel( index ); > Modules/Numerics/Statistics/test/itkSubsampleTest.cxx: > ArrayPixelImageType::PixelType pixel = filter->GetInput()->GetPixel(index); > Modules/Segmentation/Voronoi/include/itkVoronoiPartitioningImageFilter.hxx: > getp = (double)( this->GetInput()->GetPixel(Plist[i]) ); > Modules/Segmentation/Voronoi/include/itkVoronoiSegmentationImageFilter.hxx: > getp = (double)( this->GetInput()->GetPixel(Plist[i]) ); > ~/Dashboard/src/ITK (master) > > johnsonhj@neuron$ > ~/Dashboard/src/ITK (master) > johnsonhj@neuron$ git grep "\->GetInput()\->Transform" > Modules/Core/Mesh/include/itkBinaryMask3DMeshSource.hxx: > this->GetInput()->TransformContinuousIndexToPhysicalPoint(indTemp,new_p); > Modules/Nonunit/Review/include/itkScalarChanAndVeseDenseLevelSetImageFilter.hxx: > this->GetInput()->TransformPhysicalPointToIndex(origin, start); > Modules/Nonunit/Review/include/itkScalarChanAndVeseSparseLevelSetImageFilter.hxx: > this->GetInput()->TransformPhysicalPointToIndex(origin, start); > Modules/Nonunit/Review/include/itkStochasticFractalDimensionImageFilter.hxx: > this->GetInput()->TransformIndexToPhysicalPoint(It.GetIndex(i), > point1); > Modules/Nonunit/Review/include/itkStochasticFractalDimensionImageFilter.hxx: > > this->GetInput()->TransformIndexToPhysicalPoint(It.GetIndex(j), point2); > Modules/Numerics/Statistics/include/itkScalarImageToRunLengthMatrixFilter.hxx: > this->GetInput()->TransformIndexToPhysicalPoint( > Modules/Numerics/Statistics/include/itkScalarImageToRunLengthMatrixFilter.hxx: > this->GetInput()->TransformIndexToPhysicalPoint( lastGoodIndex, point > ); > ============================== > > Hans > -- > Hans J. Johnson, Ph.D. > [email protected] > Assistant Professor of Psychiatry > University of Iowa Carver College of Medicine > W278 GH, 200 Hawkins Drive > Iowa City, Iowa 52242 > Phone: 319-353-8587 > > From: Bradley Lowekamp <[email protected]> > Date: Tuesday, July 3, 2012 9:26 AM > To: ITK <[email protected]> > Cc: "[email protected]" <[email protected]>, Hans Johnson < > [email protected]>, Luis Ibanez <[email protected]> > Subject: [Insight-developers] Performance Impact of using GetInput > > Hello, > > A user yesterday, was reporting that going from ITK 3.20 to ITK 4.1, the > SignedMaurerDistanceMapImageFilter was running more that 2x-3x the time. > With a little bit of poking around and sampling the run time, I was able to > develop the following patch: > > http://review.source.kitware.com/#/c/6367/ > > I find that difference to be quite significant difference, and is on the > level of a bug. > > The lead me to wonder how wide spread is this incorrect usage. So I > added an atomic counter to the GetInput, and GetOutput methods, and when > they exceed a threshold, an exception is throw. This is to detect when > these methods may be used in an inner loop. > > http://review.source.kitware.com/#/c/6369/ > > > I get the following test failure (where previously there was none): > > > 97% tests passed, 71 tests failed out of 2382 > > The following tests FAILED: > 160 - itkN4BiasFieldCorrectionImageFilterTest1 (Failed) > 161 - itkN4BiasFieldCorrectionImageFilterTest2 (Failed) > 311 - itkMultiThreaderEnvTest88 (Failed) > 313 - itkMultiThreaderEnvTest123 (Failed) > 398 - itkFFTConvolutionImageFilterTest4x4Mean (Failed) > 399 - itkFFTConvolutionImageFilterTest4x5Mean (Failed) > 400 - itkFFTConvolutionImageFilterTest5x5Mean (Failed) > 401 - itkFFTConvolutionImageFilterTest4x4MeanValidRegion (Failed) > 402 - itkFFTConvolutionImageFilterTest4x5MeanValidRegion (Failed) > 403 - itkFFTConvolutionImageFilterTest5x5MeanValidRegion (Failed) > 420 - itkRichardsonLucyDeconvolutionImageFilterGaussianKernelTest (Failed) > 421 - itkRichardsonLucyDeconvolutionImageFilterIrregularKernelTest (Failed) > 422 - itkLandweberDeconvolutionImageFilterGaussianKernelTest (Failed) > 423 - itkLandweberDeconvolutionImageFilterIrregularKernelTest (Failed) > 425 - itkProjectedLandweberDeconvolutionImageFilterGaussianKernelTest > (Failed) > 426 - itkProjectedLandweberDeconvolutionImageFilterIrregularKernelTest > (Failed) > 427 - itkInverseDeconvolutionImageFilterGaussianKernelTest (Failed) > 428 - itkInverseDeconvolutionImageFilterIrregularKernelTest (Failed) > 429 - itkTikhonovDeconvolutionImageFilterGaussianKernelTest (Failed) > 430 - itkTikhonovDeconvolutionImageFilterIrregularKernelTest (Failed) > 431 - itkWienerDeconvolutionImageFilterGaussianKernelTest (Failed) > 432 - itkWienerDeconvolutionImageFilterIrregularKernelTest (Failed) > 433 - itkParametricBlindLeastSquaresDeconvolutionImageFilterTest (Failed) > 436 - itkDeformableSimplexMesh3DBalloonForceFilterTest (Failed) > 440 - itkPatchBasedDenoisingImageFilterTest0 (Failed) > 441 - itkPatchBasedDenoisingImageFilterTestGaussian (Failed) > 442 - itkPatchBasedDenoisingImageFilterTestRician (Failed) > 443 - itkPatchBasedDenoisingImageFilterTestPoisson (Failed) > 521 - itkDisplacementFieldToBSplineImageFilterTest (Failed) > 524 - itkContourMeanDistanceImageFilterTest (Failed) > 525 - itkContourDirectedMeanDistanceImageFilterTest (Failed) > 530 - itkHausdorffDistanceImageFilterTest (Failed) > 532 - itkSignedMaurerDistanceMapImageFilterTest1 (Failed) > 533 - itkSignedMaurerDistanceMapImageFilterTest2 (Failed) > 656 - itkFastMarchingImageFilterTest_torus_multipleSeeds_NoTopo (Failed) > 657 - itkFastMarchingImageFilterTest_torus_multipleSeeds_StrictTopo > (Failed) > 658 - itkFastMarchingImageFilterTest_torus_multipleSeeds_NoHandlesTopo > (Failed) > 659 - itkFastMarchingImageFilterTest_wm_multipleSeeds_NoTopo (Failed) > 660 - itkFastMarchingImageFilterTest_wm_multipleSeeds_StrictTopo (Failed) > 661 - itkFastMarchingImageFilterTest_wm_multipleSeeds_NoHandlesTopo > (Failed) > 1072 - itkBSplineControlPointImageFilterTest2 (Failed) > 1079 - itkCyclicShiftImageFilterTest0 (Failed) > 1080 - itkCyclicShiftImageFilterTest1 (Failed) > 1081 - itkCyclicShiftImageFilterTest2 (Failed) > 1082 - itkCyclicShiftImageFilterTest3 (Failed) > 1083 - itkCyclicShiftImageFilterTest4 (Failed) > 1084 - itkCyclicShiftImageFilterTest5 (Failed) > 1085 - itkCyclicShiftImageFilterTest6 (Failed) > 1195 - itkModulusImageFilterTest (Failed) > 1377 - itkExtensionVelocitiesImageFilterTest (Failed) > 1378 - itkCannySegmentationLevelSetImageFilterTest (Failed) > 1412 - itkTwoLevelSetsv4DenseImage2DTest (Failed) > 1471 - itkSimplexMeshVolumeCalculatorTest (Failed) > 1659 - itkBinaryMask3DQuadEdgeMeshSourceTest (Failed) > 1747 - itkPointSetToPointSetRegistrationTest (Failed) > 1774 - itkDiffeomorphicDemonsRegistrationFilterTest01 (Failed) > 1775 - itkDiffeomorphicDemonsRegistrationFilterTest02 (Failed) > 1776 - itkDiffeomorphicDemonsRegistrationFilterTest03 (Failed) > 1777 - itkDiffeomorphicDemonsRegistrationFilterTest04 (Failed) > 1778 - itkDiffeomorphicDemonsRegistrationFilterTest05 (Failed) > 1779 - itkDiffeomorphicDemonsRegistrationFilterTest06 (Failed) > 1780 - itkDiffeomorphicDemonsRegistrationFilterTest07 (Failed) > 1781 - itkDiffeomorphicDemonsRegistrationFilterTest08 (Failed) > 1782 - itkDiffeomorphicDemonsRegistrationFilterTest09 (Failed) > 1783 - itkDiffeomorphicDemonsRegistrationFilterTest10 (Failed) > 1784 - itkDiffeomorphicDemonsRegistrationFilterTest11 (Failed) > 1802 - itkFastSymmetricForcesDemonsRegistrationFilterTest (Failed) > 2166 - itkVoronoiSegmentationImageFilterTest (Failed) > > > How big of a deal if most of the filters here are running 2x+ slower > then what they should be? Is it big enough to delay the Release and do > another RC with the fixes? > > I have also been looking at the methods used in GetInput, specifically > the methods used to create the std::string... It seems to be if we change > the return value to a const std::string &, then we could keep a static > internal table of the common value and return reference to the static table > to even, references to what is in the std::map, the would reduce the need > for mallocs for std::string. > > Thoughts on what to do? > > Brad > > ======================================================== > > Bradley Lowekamp > > Medical Science and Computing for > > Office of High Performance Computing and Communications > > National Library of Medicine > > [email protected] > > > > > > ------------------------------ > Notice: This UI Health Care e-mail (including attachments) is covered by > the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is > confidential and may be legally privileged. If you are not the intended > recipient, you are hereby notified that any retention, dissemination, > distribution, or copying of this communication is strictly prohibited. > Please reply to the sender that you have received the message in error, > then delete it. Thank you. > ------------------------------ > > _______________________________________________ > Powered by www.kitware.com > > Visit other Kitware open-source projects at > http://www.kitware.com/opensource/opensource.html > > Kitware offers ITK Training Courses, for more information visit: > http://kitware.com/products/protraining.php > > Please keep messages on-topic and check the ITK FAQ at: > http://www.itk.org/Wiki/ITK_FAQ > > Follow this link to subscribe/unsubscribe: > http://www.itk.org/mailman/listinfo/insight-developers > > -- Unpaid intern in BillsBasement at noware dot com
_______________________________________________ Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Kitware offers ITK Training Courses, for more information visit: http://kitware.com/products/protraining.php Please keep messages on-topic and check the ITK FAQ at: http://www.itk.org/Wiki/ITK_FAQ Follow this link to subscribe/unsubscribe: http://www.itk.org/mailman/listinfo/insight-developers
