Bug 46811 - [PATCH] Animation engine memory leak
Summary: [PATCH] Animation engine memory leak
Status: NEW
Alias: None
Product: Batik - Now in Jira
Classification: Unclassified
Component: Animation engine (show other bugs)
Version: 1.8
Hardware: PC Windows Vista
: P2 normal
Target Milestone: ---
Assignee: Batik Developer's Mailing list
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2009-03-06 00:21 UTC by Cristi Moise
Modified: 2009-04-15 22:52 UTC (History)
0 users



Attachments
Patch from proposed changes (458 bytes, patch)
2009-03-06 02:25 UTC, Helder Magalhães
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cristi Moise 2009-03-06 00:21:52 UTC
To reproduce, succesively add, start and remove many many animation elements in a document programatically. The result is a memory leak caused by elements accumulating in two hash maps called animations and targets in the class AnimationEngine. 

Possible solution: removing elements from these two maps when an animation is removed, last two lines in the method. 

    /**
     * Removes an animation from the document.
     */
    public void removeAnimation(AbstractAnimation anim) {
        // org.apache.batik.anim.timing.Trace.enter(this, "removeAnimation", new Object[] { anim } ); try {
        timedDocumentRoot.removeChild(anim.getTimedElement());
        AbstractAnimation nextHigher = anim.higherAnimation;
        if (nextHigher != null) {
            nextHigher.markDirty();
        }
        moveToBottom(anim);
        if (anim.higherAnimation != null) {
            anim.higherAnimation.lowerAnimation = null;
        }
        AnimationInfo animInfo = getAnimationInfo(anim);
        Sandwich sandwich = getSandwich(animInfo.target, animInfo.type,
                                        animInfo.attributeNamespaceURI,
                                        animInfo.attributeLocalName);
        if (sandwich.animation == anim) {
            sandwich.animation = null;
            sandwich.lowestAnimation = null;
            sandwich.shouldUpdate = true;
        }
        animations.remove(anim);
        targets.remove(animInfo.target);
        // } finally { org.apache.batik.anim.timing.Trace.exit(); }
    }

This might not be the best solution.

I use latest version of batik sources from svn trunk repository. Version from trunk: xml-batik 746059
Comment 1 Helder Magalhães 2009-03-06 02:25:39 UTC
Created attachment 23346 [details]
Patch from proposed changes

(In reply to comment #0)
> To reproduce, succesively add, start and remove many many animation elements
> in a document programatically.

Could you please attach a test case? I haven't investigated this properly, but being able to reproduce usually helps a lot. ;-)


> Possible solution: removing elements from these two maps when an animation is
> removed, last two lines in the method. 

Stating which file was modified would avoid a find in files... :-P
The changes refer to sources/org/apache/batik/anim/AnimationEngine.java. I've created a patch to ease this. :-)

Bug 46155 and bug 46124 may also be related with this -- may this be a regression from those changes?...


Hope this helps,
 Helder Magalhães
Comment 2 Cristi Moise 2009-03-06 04:18:08 UTC
> Could you please attach a test case? I haven't investigated this properly, but
> being able to reproduce usually helps a lot. ;-)

A possible test case would look like this:

Element[] animatedElements = new Element[100];
while(true){	
	//start 100 animations
	for(int i=0;i<100;i++){
		//create the element to be animated
		Element animatedElement = document.createElementNS("http://www.w3.org/2000/svg", "rect");	
		animatedElement.setAttributeNS(null, "x", "0");
		animatedElement.setAttributeNS(null, "y", "0");		
		animatedElement.setAttributeNS(null, "width", "100");
		animatedElement.setAttributeNS(null, "height", "100");
		animatedElement.setAttributeNS(null, "style", "fill:red");
	
		// add animation to the animatedElement
		Element animation = document.createElementNS("http://www.w3.org/2000/svg", "animateMotion");	
		animation.setAttributeNS(null, "begin", "indefinite");
		animation.setAttributeNS(null, "dur", "1s");
		animation.setAttributeNS(null, "fill", "freeze");
		animation.setAttributeNS(null, "path", "M 0 0 L 100 100");
									
		// add animation to animatedElement
		animatedElement.appendChild(animation);
		// add the animated element to the document
		((SVGDocument) document).getRootElement().appendChild(animatedElement);				
		// start the animation now
		((ElementTimeControl) animation).beginElement();
		//remember the animated element so that we can later remove it
		animatedElements[i]=animatedElement;
	}
	Thread.sleep(10);
	//remove the above created 100 animated elements
	for(int i=0;i<100;i++){			
		//remove animated element, this also removes the animation
		animatedElements[i].getParentNode().removeChild(animatedElements[i]);	
	}
}

> Stating which file was modified would avoid a find in files... :-P
I did mention the name of the class AnimationEngine, sorry I didn't put it so that it's easier to see, it's the first time I submit a bug, ever as far as I can remember.

> Bug 46155 and bug 46124 may also be related with this -- may this be a
> regression from those changes?...

I really don't know the details of batik developement or how things work in this complex library, I just profiled and found the place the memory leak happened and fixed the sources, I don't know if it's a good solution. Just needed a quick fix four our application, and tought to help you out a little to improve the libs.

Best regards,
Cristi.
Comment 3 Helder Magalhães 2009-04-15 22:52:06 UTC
(In reply to comment #2)
> > Stating which file was modified would avoid a find in files... :-P
> I did mention the name of the class AnimationEngine, sorry I didn't put it so
> that it's easier to see, it's the first time I submit a bug, ever as far as I
> can remember.

Sorry, I didn't notice the class name. Of course that was sufficient. ;-)
Thanks for the report! :-)


@Batik developers:
Patch seems simple and safe to apply. Nevertheless, the proposed fix may be simply obfuscating something deeper... I'm also still not certain if this wasn't caused by any of the mentioned fixes (see comment 1).

Also, do you believe it's worth to wrap up a stand-alone SVG test case (based on the one in comment 2)? Maybe even try to integrate it into regard's memory leak tests...?