Bill,<div><br></div><div>Now I&#39;m at home. You will have the results of the experiment tomorrow morning.</div><div><br></div><div>Thanks,</div><div><br></div><div>Roger<br><br><div class="gmail_quote">On Tue, Mar 23, 2010 at 8:44 PM, Bill Lorensen <span dir="ltr">&lt;<a href="mailto:bill.lorensen@gmail.com">bill.lorensen@gmail.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Sounds good. So you are saying that in the<br>
ImageSeriesReader&lt;TOutputImage&gt;<br>
::GenerateOutputInformation<br>
<br>
that the number of files that are processed will be only 2.<br>
<br>
Bill<br>
<br>
On Tue, Mar 23, 2010 at 12:39 PM, Bradley Lowekamp<br>
<div><div></div><div class="h5">&lt;<a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a>&gt; wrote:<br>
&gt; Bill:<br>
&gt; My proposed solution is to use the old behavior but use a time stamp to<br>
&gt; avoid the extra updates of the MDDA when streaming.<br>
&gt; The requested region is set after UpdateOutputInformation is executed, so it<br>
&gt; can&#39;t be toggled during the this phase of execution.<br>
&gt;<br>
&gt; On Mar 23, 2010, at 3:28 PM, Bill Lorensen wrote:<br>
&gt;<br>
&gt; Can we check if streaming is on and revert to the old behavior if it is off?<br>
&gt;<br>
&gt; This is a huge performance penalty to support streaming which is<br>
&gt; important but no the usual use case.<br>
&gt;<br>
&gt;<br>
&gt; On Tue, Mar 23, 2010 at 12:12 PM, Bradley Lowekamp<br>
&gt; &lt;<a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a>&gt; wrote:<br>
&gt;<br>
&gt; Bill,<br>
&gt;<br>
&gt; Going back would have horrible effects for streaming. It would make slice by<br>
&gt;<br>
&gt; slice streaming an n^2 algorithm, which is far worse then the current order<br>
&gt;<br>
&gt; of N hindrance for normals Updates. We must make some improvements from 2.8.<br>
&gt;<br>
&gt; If we declare the the MetaDataDictionary is suppose to be updated in the<br>
&gt;<br>
&gt; update data phase. ( the ImageFileReader does it in the<br>
&gt;<br>
&gt; UpdateOutputInformation phase ) Then the prior stated point 1 design<br>
&gt;<br>
&gt; requirement is gone. And the following solution come to mind:<br>
&gt;<br>
&gt; 1) Modify the GetMMDA methods to produce a warning if the update output data<br>
&gt;<br>
&gt; has not been called. This is to be nice if some users now expect<br>
&gt;<br>
&gt; UpdateOutputInformation to produce the MDDA.<br>
&gt;<br>
&gt; 2) Add a time stamp for the MMDA, so that when streaming the MMDA is only<br>
&gt;<br>
&gt; updated once and not every time a region is requested.<br>
&gt;<br>
&gt; Additionally I believe that we need better DICOM test data which include<br>
&gt;<br>
&gt; more tags similar to real world data.<br>
&gt;<br>
&gt; Brad<br>
&gt;<br>
&gt; On Mar 23, 2010, at 2:54 PM, Bill Lorensen wrote:<br>
&gt;<br>
&gt; The UpdateInformation is supposed to update origin, spacing,<br>
&gt;<br>
&gt; direction, pixel type, etc. I don&#39;t think it is supposed to completely<br>
&gt;<br>
&gt; populate the meta data dictionary. At least until itk 2.8 it did not.<br>
&gt;<br>
&gt; Why not revert back to the old behavior as a sort term fix.<br>
&gt;<br>
&gt; I think this performance hit needs to be repaired before we release<br>
&gt;<br>
&gt; 3.16. This has been causing major pain for Slicer3 users who<br>
&gt;<br>
&gt; frequently use dicom. Fortunately for us, Roger brought it to light.<br>
&gt;<br>
&gt; We missed it because our performance testing is weak.<br>
&gt;<br>
&gt; There are other issues for sure.<br>
&gt;<br>
&gt; Bill<br>
&gt;<br>
&gt; On Tue, Mar 23, 2010 at 11:45 AM, Bradley Lowekamp<br>
&gt;<br>
&gt; &lt;<a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a>&gt; wrote:<br>
&gt;<br>
&gt; Bill,<br>
&gt;<br>
&gt; After my tests I agree that reading the headers in DICOM files is a<br>
&gt;<br>
&gt; surprisingly expensive operation as such it should be minimized. The coping<br>
&gt;<br>
&gt; of the MDAs is insignificant performance wise.  I believe that the best<br>
&gt;<br>
&gt; solution would be to have a dedicated DICOM series readers, which also<br>
&gt;<br>
&gt; removes the extra header reads needed for the name generation as well as the<br>
&gt;<br>
&gt; extra one in the UpdateOutputInformation.<br>
&gt;<br>
&gt; If we assume that the usually way to utilize the reader is to just Update,<br>
&gt;<br>
&gt; or stream Update, then the additional read of the headers appears<br>
&gt;<br>
&gt; unnecessary.<br>
&gt;<br>
&gt; I believe a solution would be to make the GetMDDA method smarter, and by<br>
&gt;<br>
&gt; default update this MDDA in the UpdateData. A time stamp would need to be<br>
&gt;<br>
&gt; used for the MDDA to check when it needs to be updated in the UpdateData<br>
&gt;<br>
&gt; methods. For streaming, the first time through would require reading all of<br>
&gt;<br>
&gt; the headers for the MDDA, this should bring the time stamp up to date. The<br>
&gt;<br>
&gt; GetMDDA methods could also check this timestamp and perform the reading of<br>
&gt;<br>
&gt; the headers if it&#39;s out of date. This is my best current idea on how to<br>
&gt;<br>
&gt; maintain the 1) and 2) I previously mentioned.<br>
&gt;<br>
&gt; Brad<br>
&gt;<br>
&gt; On Mar 23, 2010, at 12:33 PM, Bill Lorensen wrote:<br>
&gt;<br>
&gt; Brad,<br>
&gt;<br>
&gt; I have an itk 2.8 checkout. The difference is due to the processing of<br>
&gt;<br>
&gt; all files in the GenerateOutputInformation method. In the past, only<br>
&gt;<br>
&gt; two files were processed. If I restrict the number of files to 2<br>
&gt;<br>
&gt; rather that number of files, I get pretty reasonable speeds.<br>
&gt;<br>
&gt; Roger,<br>
&gt;<br>
&gt; As an experiment (and definitely not a fix!), can you in the method<br>
&gt;<br>
&gt; void ImageSeriesReader&lt;TOutputImage&gt;<br>
&gt;<br>
&gt; ::GenerateOutputInformation(void)<br>
&gt;<br>
&gt; change the line:<br>
&gt;<br>
&gt; for ( int i = 0; i != numberOfFiles; ++i )<br>
&gt;<br>
&gt; to<br>
&gt;<br>
&gt; for ( int i = 0; i != 2; ++i )<br>
&gt;<br>
&gt; and rerun your tests.<br>
&gt;<br>
&gt; Bill<br>
&gt;<br>
&gt;<br>
&gt; On Tue, Mar 23, 2010 at 8:59 AM, Bradley Lowekamp<br>
&gt;<br>
&gt; &lt;<a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a>&gt; wrote:<br>
&gt;<br>
&gt; Bill,<br>
&gt;<br>
&gt; That is only the half of it. Every time an ImageFileReader is used 3 MDDs<br>
&gt;<br>
&gt; (meta data dictionaries) are created, one in the ImageIO, one in the<br>
&gt;<br>
&gt; ImageFileReader, and one in the output Image. This is in addition to the two<br>
&gt;<br>
&gt; copies, you pointed out in ImageSeriesReader. Clearly reading with an<br>
&gt;<br>
&gt; ImageFileReader the MDD scales very poorly as the it&#39;s size increases. I<br>
&gt;<br>
&gt; still have the remaining performance questions:<br>
&gt;<br>
&gt; How much time is spent coping the MDD vs reading? (leaning towards reading<br>
&gt;<br>
&gt; as very expensive)<br>
&gt;<br>
&gt; As pointed out in Roger&#39;s most recent performance tests, there appears to be<br>
&gt;<br>
&gt; some additional performance problems in the UpdateData, part. This is<br>
&gt;<br>
&gt; independent of the additional MDD read in the UpdateOutputInformation. This<br>
&gt;<br>
&gt; is definitely another problem, perhaps inside the DICOM library.<br>
&gt;<br>
&gt; The change of moving (apparently duplicating) the copying to MDDs to the MDD<br>
&gt;<br>
&gt; array was added over a year ago, when streaming support was added. If I<br>
&gt;<br>
&gt; recall correctly the two motivating factors were 1) the MDD array is output<br>
&gt;<br>
&gt; information and logically should be updating during the<br>
&gt;<br>
&gt; UpdateOutputInformation part of the pipeline 2) when streaming each file<br>
&gt;<br>
&gt; should not need to be read to create the MMD array. I don&#39;t recall where<br>
&gt;<br>
&gt; this discussion took place right now.<br>
&gt;<br>
&gt; I will run some performance test to try to figure out where the time is<br>
&gt;<br>
&gt; being spent. Without changing 1 from above, I am not sure how much could be<br>
&gt;<br>
&gt; gained.<br>
&gt;<br>
&gt; Looking at the performance numbers of the Read Directory part, I would guess<br>
&gt;<br>
&gt; that the meta data is also read there. I believe that an idea solution would<br>
&gt;<br>
&gt; only read this information once. But that is beyond this scope.<br>
&gt;<br>
&gt; Brad<br>
&gt;<br>
&gt; On Mar 22, 2010, at 11:20 PM, Bill Lorensen wrote:<br>
&gt;<br>
&gt; Brad,<br>
&gt;<br>
&gt; It looks like the meta data array is populated in both the<br>
&gt;<br>
&gt; GenerateOutputInformation and GenerateData. Also all slices are<br>
&gt;<br>
&gt; processed in GenerateOutputInformation. In 2.8, only 2 slices were<br>
&gt;<br>
&gt; processed.<br>
&gt;<br>
&gt; Why were these changes made? We are also seeing bad dicom performance<br>
&gt;<br>
&gt; in Slicer3.<br>
&gt;<br>
&gt; Bill<br>
&gt;<br>
&gt; On Mon, Mar 22, 2010 at 6:24 AM, Bradley Lowekamp<br>
&gt;<br>
&gt; &lt;<a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a>&gt; wrote:<br>
&gt;<br>
&gt; Hello,<br>
&gt;<br>
&gt; Can you please tell us a little more about your test data and computer. What<br>
&gt;<br>
&gt; kind of file system is the data on ( locale or network)? How much memory<br>
&gt;<br>
&gt; does the computer have? What is the size of the data? What is the native<br>
&gt;<br>
&gt; pixel type of the data? What are the actual timings? Does the execution seem<br>
&gt;<br>
&gt; to be CPU or IO bound?<br>
&gt;<br>
&gt; One of the changes that was made to the class was to populate the<br>
&gt;<br>
&gt; MetaDataArray in the UpdataOutputInformation phase of the instead of the<br>
&gt;<br>
&gt; UpdateOutputData part. This should be just reading the headers of the files<br>
&gt;<br>
&gt; in the series. There were several reasons this change was made. To help<br>
&gt;<br>
&gt; determine the cause of your slowness, lets break up the timing a little<br>
&gt;<br>
&gt; further.<br>
&gt;<br>
&gt; Could you please call:<br>
&gt;<br>
&gt; start timer<br>
&gt;<br>
&gt; reader-&gt;UpdateOutputInformation();<br>
&gt;<br>
&gt; lap timer<br>
&gt;<br>
&gt; reader-&gt;UpdateLargestPossibleRegion();<br>
&gt;<br>
&gt; stop timer<br>
&gt;<br>
&gt; And post the timing results.<br>
&gt;<br>
&gt; Thanks,<br>
&gt;<br>
&gt; Brad<br>
&gt;<br>
&gt; On Mar 21, 2010, at 2:52 PM, Roger Bramon Feixas wrote:<br>
&gt;<br>
&gt; This week we updated our ITK version from 2.8 to 3.16  and we noticed the<br>
&gt;<br>
&gt; medical models are loading 2x slower using the 3.16 ITK version. We use<br>
&gt;<br>
&gt; itk::ImageSeriesReader and the problem is focused in its Update() method.<br>
&gt;<br>
&gt; I attached a simple test program which reproduces the problem and where we<br>
&gt;<br>
&gt; can see that the Update() method is 2 times slower using ITK 3.16 vs. ITK<br>
&gt;<br>
&gt; 2.8.<br>
&gt;<br>
&gt; We compiled both versions using Visual Studio 2008 on Windows XP 32bits and<br>
&gt;<br>
&gt;  we don&#39;t known if this problem also occurs in other platforms.<br>
&gt;<br>
&gt; I wonder if other itk users have this same performance problem and if there<br>
&gt;<br>
&gt; is anybody can help us in order to solve it.<br>
&gt;<br>
&gt; Thanks!<br>
&gt;<br>
&gt; Roger<br>
&gt;<br>
&gt;<br>
&gt; ========================================================<br>
&gt;<br>
&gt; Bradley Lowekamp<br>
&gt;<br>
&gt; Lockheed Martin Contractor for<br>
&gt;<br>
&gt; Office of High Performance Computing and Communications<br>
&gt;<br>
&gt; National Library of Medicine<br>
&gt;<br>
&gt; <a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a><br>
&gt;<br>
&gt;<br>
&gt;<br>
&gt; ========================================================<br>
&gt;<br>
&gt; Bradley Lowekamp<br>
&gt;<br>
&gt; Lockheed Martin Contractor for<br>
&gt;<br>
&gt; Office of High Performance Computing and Communications<br>
&gt;<br>
&gt; National Library of Medicine<br>
&gt;<br>
&gt; <a href="mailto:blowekamp@mail.nih.gov">blowekamp@mail.nih.gov</a><br>
&gt;<br>
&gt;<br>
</div></div></blockquote></div><br></div>