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
>> [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-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();
>>>>>>>>> }
>>