Vincent Massol wrote: 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.

On Mon, Mar 1, 2010 at 11:17, Vincent Massol <vincent(a)massol.net> wrote: Yes this seems very wrong, the only place i can see is Utils#componentManager but this is cleaned in tearDown() already. -- Thomas Mortagne

On Mar 1, 2010, at 11:42 AM, Caleb James DeLisle wrote: 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

Hi Caleb, On Mar 3, 2010, at 7:36 AM, Caleb James DeLisle wrote: * 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. * 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). * BTW2: We could easily have the fork only in xwiki-core and have all other build modules use non forked tests. * I don't understand why 1 would take more memory than 3. * 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. Thanks -Vincent

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? 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 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 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: >>> >>> And with the tests forked (current setup) >>> >>> 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-patchTestMemo… >>>>>>>>> >>>>>>>>> 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 >> devs(a)xwiki.org >> http://lists.xwiki.org/mailman/listinfo/devs >> > > _______________________________________________ > devs mailing list > devs(a)xwiki.org > http://lists.xwiki.org/mailman/listinfo/devs >

On Sat, Mar 6, 2010 at 06:22, Caleb James DeLisle &lt;calebdelisle(a)lavabit.com&gt; wrote: Sounds good. -- Thomas Mortagne