Bug 143277 - ElementImpl.getDeclaration should store return value in instance field as its called many many times , improves validation time by 40%
Summary: ElementImpl.getDeclaration should store return value in instance field as its...
Status: NEW
Alias: None
Product: WTP Source Editing
Classification: WebTools
Component: wst.xml (show other bugs)
Version: 1.5   Edit
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: Future   Edit
Assignee: Nick Sandonato CLA
QA Contact: Nick Sandonato CLA
URL:
Whiteboard:
Keywords: performance
Depends on:
Blocks:
 
Reported: 2006-05-23 13:53 EDT by Raj Mandayam CLA
Modified: 2013-06-19 11:13 EDT (History)
4 users (show)

See Also:


Attachments
test patch (16.28 KB, patch)
2010-02-23 23:22 EST, Nitin Dahyabhai CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raj Mandayam CLA 2006-05-23 13:53:41 EDT
Well we are profiling validation of this project with 200 jsp's on WTP 1.5 RC3

Noticed that 

JSPModelQueryImpl.getCMElementDeclaration(..) was taking nearly 21 seconds out of 67 seconds taken in validating a project

This method is called from

1. CMNodeUtil.getDeclaration (ElementImpl.getDeclaration)

and

2. CMUtil.getDeclaration



Found out that most of the cases the value returned by these methods don't change per ElementImpl, 

so it should be cached or stored in an instance variable of ElementImpl

I did that by adding an instance variable in ElementImpl.java

	protected CMElementDeclaration getDeclaration() {
		if (fCMElementDeclaration == null) {
			Document document = getOwnerDocument();
			if (document == null)
				fCMElementDeclaration =  null;
			ModelQuery modelQuery = ModelQueryUtil.getModelQuery(document);
			if (modelQuery == null)
				fCMElementDeclaration =  null;
			fCMElementDeclaration = modelQuery.getCMElementDeclaration(this);
			
		}
		return fCMElementDeclaration;
		
	}


then made CMUtil.getDeclaration

call CMNodeUtil.getDeclaration

which calls ElementImpl.getDeclaration

by lazy initializing it and storing it

now time taken by the method  JSPModelQueryImpl.getCMElementDeclaration(..)

went down to 3.6 seconds

and validation went down to 39 seconds.
Comment 1 Raj Mandayam CLA 2006-09-18 18:08:45 EDT
Is there any updates on this defect ? Just upping the severity so that it gets a look.
Comment 2 David Williams CLA 2006-09-18 19:01:48 EDT
Ok, Raj ... you got my attention ... but, you can't up the severity just to get my attention! :) 

You could just ask a polite question here, and would be get just as much positive attention and less negative. 

The problem with caching content model related information is its then hard to know when to clear the cache. When a URI or name space changes? When some change the content model settings in file preferences? I'm not saying its imposible, but just not easy .. which I suspect is why we don't already do it. 

So, we will look at ... but, a fix for 1.5.1 is highly unlikely simply due to lack of time and resources. 

Comment 3 Raj Mandayam CLA 2006-09-18 20:55:02 EDT
oh, sorry abt that, I was going through a list of defects opened with no response yet, and routinely increased it severity. will be more careful next time.
Comment 4 Gary Karasiuk CLA 2008-07-14 14:18:54 EDT
Hi Valentin, Do you have an update on this one? 
Comment 5 Valentin Baciu CLA 2008-07-14 16:54:14 EDT
Hi Gary, no update - it's the first time I read this bug. I would think that David's thoughts in comment #2 are still valid.
Comment 6 Gary Karasiuk CLA 2008-07-14 17:07:25 EDT
(In reply to comment #5)
> Hi Gary, no update - it's the first time I read this bug. I would think that
> David's thoughts in comment #2 are still valid.
> 
The thought about looking at it again? :-)  

I don't know how big this cache would get, but clearing at the end of the validation might be appropriate. 
Comment 7 Nitin Dahyabhai CLA 2008-07-23 06:47:18 EDT
Reassigning to my appointed performance contact.  I know CMUtil.getDeclaration
 was called repeatedly in the HTML element content validator for the same element until *very* recently, but CMNodeUtil.getDeclaration doesn't look familiar.
Comment 8 Ian Tewksbury CLA 2010-02-23 15:19:02 EST
Nitin, is this something you still want to do?
Comment 9 Nitin Dahyabhai CLA 2010-02-23 23:22:59 EST
Created attachment 160028 [details]
test patch

Attaching a "test patch", which caches the lookups inside the ModelQuery during validation.  Informal measurements don't show it improving times all that much--maybe 5-9%.  And I haven't profiled with this patch to see if any new areas stick out.