On Sat, Mar 6, 2010 at 06:22, Caleb James DeLisle <[email protected]> wrote: > After running some more tests I find that 13 and 34 minutes were > both outliers. I found on Hudson that the last 10 builds averaged > 16.1 minutes each, I would like to make this change and then revisit > the issue after the next 10 builds. > > WDYT?
Sounds good. > > Caleb > > > Some more statistics: > > With the tests forked (current setup) > > [INFO] Total time: 27 minutes 37 seconds > [INFO] Finished at: Thu Mar 04 11:22:56 GMT-05:00 2010 > [INFO] Final Memory: 169M/496M > > [INFO] Total time: 23 minutes 33 seconds > [INFO] Finished at: Thu Mar 04 09:10:04 GMT-05:00 2010 > [INFO] Final Memory: 173M/496M > > [INFO] Total time: 21 minutes 22 seconds > [INFO] Finished at: Thu Mar 04 13:31:52 GMT-05:00 2010 > [INFO] Final Memory: 188M/496M > >>>> [INFO] Total time: 34 minutes 58 seconds >>>> [INFO] Finished at: Wed Mar 03 01:02:29 GMT-05:00 2010 >>>> [INFO] Final Memory: 167M/496M > > > > > With patch for clearing context: > [INFO] Total time: 20 minutes 45 seconds > [INFO] Finished at: Wed Mar 03 03:44:52 GMT-05:00 2010 > [INFO] Final Memory: 182M/496M > > [INFO] Total time: 20 minutes 32 seconds > [INFO] Finished at: Sat Mar 06 00:16:52 GMT-05:00 2010 > [INFO] Final Memory: 193M/496M > > > > With memory increased. > [INFO] Total time: 22 minutes 59 seconds > [INFO] Finished at: Wed Mar 03 06:19:45 GMT-05:00 2010 > [INFO] Final Memory: 191M/496M > >>>> [INFO] Total time: 13 minutes 41 seconds >>>> [INFO] Finished at: Wed Mar 03 00:20:01 GMT-05:00 2010 >>>> [INFO] Final Memory: 96M/218M > > > Caleb James DeLisle wrote: >> 2 doesn't require any increased memory, just clearing the context in >> the teardown. >> >> Vincent Massol wrote: >>> Hi Caleb, >>> >>> On Mar 3, 2010, at 7:36 AM, Caleb James DeLisle wrote: >>> >>>> As I see it there are three known workarounds for the problem at the >>>> moment. >>>> 1. Isolate the tests - <forkMode>pertest</forkMode> >>>> 2. Clear the context (patch). >>>> 3. Increase the memory for tests - <argLine>-Xmx512m</argLine> >>> * What about 2? Does 2 require increased memory too? >>> * BTW we all agree about reducing build times. It is *critical* to have the >>> fastest possible build. >> Great, I guess I was just feeling cynical when i thought "make it >> slow and someone will fix it". >> >>> * I'm really surprised that the time difference is so important. I remember >>> I had done some tests when I put the forktest and the differences were very >>> small which is why we did it (maybe we added a lot of tests since then, >>> causing the problem). >> I always remember 1/2 hour, maybe it's my computer. >> >>> * BTW2: We could easily have the fork only in xwiki-core and have all other >>> build modules use non forked tests. >> I did a find-grep and xwiki-core is the only place I find 'pertest' >> >>> * I don't understand why 1 would take more memory than 3. >> Not sure either, one guess is starting and stopping the vm. >> >>> * Personally, as I've already said, I'm for 2 if it solves the memory >>> issue and allows to run tests in non fork mode. As a second option I'm for >>> 3. >> "solves" might be too strong a word :) but it runs with the default >> memory settings. >> >> I have to build again, I'll try the clear context patch and report >> the time building that. >> >> >> Thanks for the interest, >> >> Caleb >> >> >>> Thanks >>> -Vincent >>> >>>> Here is a test with the memory increased: >>>> [INFO] Total time: 13 minutes 41 seconds >>>> [INFO] Finished at: Wed Mar 03 00:20:01 GMT-05:00 2010 >>>> [INFO] Final Memory: 96M/218M >>>> >>>> And with the tests forked (current setup) >>>> [INFO] Total time: 34 minutes 58 seconds >>>> [INFO] Finished at: Wed Mar 03 01:02:29 GMT-05:00 2010 >>>> [INFO] Final Memory: 167M/496M >>>> >>>> both built from platform/trunks with mvn clean install -Pdev >>>> >>>> I think the options listed above are (IMO) comparable in terms of >>>> 'hackishness' with the only difference that one takes almost three >>>> times as long to compile. >>>> >>>> It seems that the viewpoints on this are: >>>> 1. Cob it together for now and hope the leaks get fixed or retired >>>> with the old core. >>>> 2. Make it take a long time to compile in the hope that someone will >>>> get disgusted enough to route out some of the leaks. >>>> >>>> I favor 1 because the old core is deprecated so those leaks' days >>>> are numbered anyway, and also when platform takes a half hour to >>>> build, contributors will find it harder to make and test patches. >>>> >>>> Ok, I will stop beating this dead horse now. >>>> Caleb >>>> >>>> >>>> Vincent Massol wrote: >>>>> On Mar 1, 2010, at 11:42 AM, Caleb James DeLisle wrote: >>>>> >>>>>> Vincent Massol wrote: >>>>>>> On Mar 1, 2010, at 11:18 AM, Caleb James DeLisle wrote: >>>>>>> >>>>>>>> Vincent Massol wrote: >>>>>>>>> On Mar 1, 2010, at 10:48 AM, Caleb James DeLisle wrote: >>>>>>>>> >>>>>>>>>> By clearing the XWikiContext in the tearDown of the unit test, I can >>>>>>>>>> stop the memory leak and stop running the tests in isolation. I am >>>>>>>>>> proposing we apply this change because the XWikiContext is created >>>>>>>>>> per test and the worst problem which can be created is tests >>>>>>>>>> erroneously passing which I don't think is very likely from a change >>>>>>>>>> like this. >>>>>>>>> +1 for the main reason that this improves build times. The second >>>>>>>>> reason is that it makes memory leaks more visible which gives us a >>>>>>>>> better chance to find them (although this won't replace performance >>>>>>>>> tests running over a few days for ex). >>>>>>>>> >>>>>>>>> However please add a comment just above the getContext().clear() to >>>>>>>>> explain why this is needed. BTW why is it needed? :) >>>>>>>> I will do that. The reason it's needed is because (it seems) the >>>>>>>> context is referenced by a static variable which isn't cleared and >>>>>>>> rather than pick through the tests looking for where the reference >>>>>>>> was (and hoping no test ever does it again) I opted to clear the >>>>>>>> context and let the tests use the context as they wish. This is my >>>>>>>> understanding of the situation. >>>>>>> Then the question is why do we reference the context statically? Can't >>>>>>> we remove that? >>>>>> Static referencing is more a theory than a fact, the problem as I >>>>>> see it is the context is used everywhere and referenced in a lot of >>>>>> places so anything which is able to escape the tearDown process and >>>>>> happens to hold a reference to the context will cause references to >>>>>> persist pointing to everything. I think the choice is to either >>>>>> clear the context or pour through each test individually looking for >>>>>> any reference which might escape tearDown. I remember running >>>>>> various tests individually and in pairs with very little memory and >>>>>> finding a lot of them seem to have leaks. >>>>> Well the GC doesn't run all the time so you need to ensure you're forcing >>>>> it to run after a test to know if the test has a leak. >>>>> >>>>> And yes, I couldn't get to the bottom of it in the past (didn't spend >>>>> enough time) which is why I took the easy route of forking tests to >>>>> isolate them. >>>>> >>>>> Thanks >>>>> -Vincent >>>>> >>>>>>>>>> Jira issue: http://jira.xwiki.org/jira/browse/XWIKI-4953 >>>>>>>>>> Patch: >>>>>>>>>> http://jira.xwiki.org/jira/secure/attachment/16788/XWIKI-4496-patchTestMemoryLeak.patch >>>>>>>>>> >>>>>>>>>> Patch content: >>>>>>>>>> >>>>>>>>>> Index: core/xwiki-core/pom.xml >>>>>>>>>> =================================================================== >>>>>>>>>> --- core/xwiki-core/pom.xml (revision 27357) >>>>>>>>>> +++ core/xwiki-core/pom.xml (working copy) >>>>>>>>>> @@ -823,10 +823,6 @@ >>>>>>>>>> <plugin> >>>>>>>>>> <groupId>org.apache.maven.plugins</groupId> >>>>>>>>>> <artifactId>maven-surefire-plugin</artifactId> >>>>>>>>>> - <configuration> >>>>>>>>>> - <!-- Prevent Out Of Memory errors resulting from tests >>>>>>>>>> that do no free up the memory correctly --> >>>>>>>>>> - <forkMode>pertest</forkMode> >>>>>>>>>> - </configuration> >>>>>>>>>> </plugin> >>>>>>>>>> <!-- Publish a Core Test JAR --> >>>>>>>>>> <plugin> >>>>>>>>>> Index: >>>>>>>>>> core/xwiki-core/src/test/java/com/xpn/xwiki/test/AbstractBridgedXWikiComponentTestCase.java >>>>>>>>>> =================================================================== >>>>>>>>>> --- >>>>>>>>>> core/xwiki-core/src/test/java/com/xpn/xwiki/test/AbstractBridgedXWikiComponentTestCase.java >>>>>>>>>> (revision 27357) >>>>>>>>>> +++ >>>>>>>>>> core/xwiki-core/src/test/java/com/xpn/xwiki/test/AbstractBridgedXWikiComponentTestCase.java >>>>>>>>>> (working copy) >>>>>>>>>> @@ -80,6 +80,7 @@ >>>>>>>>>> protected void tearDown() throws Exception >>>>>>>>>> { >>>>>>>>>> Utils.setComponentManager(null); >>>>>>>>>> + getContext().clear(); >>>>>>>>>> super.tearDown(); >>>>>>>>>> } >>> _______________________________________________ >>> devs mailing list >>> [email protected] >>> http://lists.xwiki.org/mailman/listinfo/devs >>> >> >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs >> > > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs > -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
