Bugzilla – Full Text Bug Listing
| Summary: | XMLCatalogIdResolver does too much work | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Source Editing | Reporter: | Nitin Dahyabhai <thatnitind> | ||||||
| Component: | wst.xml | Assignee: | Valentin Baciu <valentinbaciu> | ||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P2 | CC: | csalter, david_williams, karasiuk, laffrac, lmandel, ramanday | ||||||
| Version: | 1.5 | ||||||||
| Target Milestone: | 2.0 RC0 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | PMC_deferred | ||||||||
| Attachments: |
|
||||||||
Thanks for looking into this Nitin. I'm assigning to me to look this over. I'll be distracted until the end of the week but I should have time to dig into this next week. This fix is crucial to improving performance of the various validators using ModelManagerImpl.getModelForRead API without this fix html validator was taking 57 seconds on a project with this fix html validator was taking 37 seconds on a project. nearly 36% improvement Craig, any progress? We are fast approaching RC1. Hi Craig, The fix seems safe, and as we had looked the resolver was not using the base location as well. I believe the approach you suggest would prevent the source editor from utilizing the other 'goodness' from the URIResolver extensions. For example caching would no longer work if we simply call out directly to the XMLCatalogExtension. I know Lawrence put a lot of work into making J2EE descriptor editing utilize cached schemas (that originate from SUN). I'm guessing the change you suggest would break that (correct me if I'm missing something though). If so I think we need to invetigate an alternative fix. I need to profile to see exactly where time is being eaten. If it is happening in the 'uri -> IFile' conversion then one possible fix would be to use a ctor like this... ExtensibleURIResolver(IFile file) ... so we can pass the IFile in and skip the conversion expense. BTW, is there a JUnit test case I can use to reproduce the timing information? If not Raj, can you contribute one? This would not only help us fix this, but we could also help ensure the html validator perfromance doesn't regress with future changes. At the very least can you attach HTML files that can be used to reproduce? FYI, we've had other "complaints" that implicate this method. See bug 146032. We have, I think, found a work around for the most frequent cases (leading the complaint) for 1.5 release, but seems we're fixing at the wrong level. Also, you asked for JUnit tests. Bug 146032 mentions some of them, but I'll repeat here. Try Open100KBFileTestCase.java Open200KBFileTestCase.java Open500KBFileTestCase.java (you can just select the class in navigator and "run as JUnit Plugin test"). It should print out summary in your Eclipse console. Then, in XHTMLAssociationProvider, set the USE_QUICK_CACHE constant to false, to avoid using any cache, so it executes resolve on each method call: grammerURI = idResolver.resolve(null, publicId, systemId); We do, btw, call this method in some of these scenerios 10's of 1000's of times, since we want to look up the content model for each Node as we are parsing a DOM tree. So, granted, there is maybe improvements we could make there ... but ... that will take some care (since DOM's can have mixed content models), and its just surprising that this one little, frequently called method would contribute so dramatically to these test cases. (When I do the above tests in my dev. env., I see improvements on the order of 20% to 50%. For small DOM trees, I doubt the difference would be noticable. Some profiling shows this is a (continuing) big problem, so I think worth addressing in 1.5.3. I was profiling running html validation on a large workspace we found a lot of time was spent in converting the String baseLocation to IFile (infact somehow the code that was getting called was like
If you look at where time was spent,
its in ExtensibleUIRResolver.computeFile(String) calling WorkspaceRoot.getFileForLocation(...)
The following code was repeatedly called as well.
CMDocument cmDocument = getCMDocument("", "", "DTD"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
I am trying to create test files so that you can fix it for 153
Valentin, please work with Raj to investigate this problem. I finally got some free time to focus on this bug. I managed to set-up the (huge) workspace provided by Raj and did an initial profiling session to try to confirm Raj's results. I then decided to do some stepping through the code in debug mode to try to figure out what's going on, since this part of the code is a little foreign to me. At first look, it seems to me that we could avoid altogether the need to invoke the URIResolver repeatedly as we do know, and this way avoid most of the performance penalty currently observed. Here's what seems to be going on here: 1. As David mentions in his comment, the XHTMLAssociationProvider#getXHTMLCMDocument gets called for every node in an HTML document. 2. This in turn calls XHTMLAssociationProvider.getXHTMLCMDocument(publicID, systemID) which seems to perform the URI resolution for the system ID and cache it. (the USE_QUICK_CACHE flag David mentions). The public ID is -//W3C//DTD XHTML 1.0 Transitional//EN and system id is http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd. Both of these point to something that amounts to a static content model document (DTD) for XHTML. 3. Next XHTMLAssociationProvider tries to get the CM document by calling CMDocumentManagerImpl.getCMDocument(String, String, String). At this point keep in mind that the URI for the XHTML CM document was already resolved to the real workspace location (as contributed to the XML catalog). Lacking this information, the code goes on and calls CMDocumentManagerImpl.lookupOrCreateResolvedURI(String, String) which, because PROPERTY_USE_CACHED_RESOLVED_URI is always false will yet again call the URIResolver to try resolve the URI. And this happens FOR EVERY NODE in document! And so it seems to me that the smarts of caching the resolved CM document URI in XHTMLAssociationProvider takes only a half bite out of the problem because it does not know that CMDocumentManagerImpl will go on and try to re-resolve the URI. There seem to be at least two ways of working around this problem in the 1.5.3 timeframe: 1. Figure out the reason why the CMDocumentManagerImpl's URI cache is not used - it seems the PROPERTY_USE_CACHED_RESOLVED_URI property is deliberately set to false in the constructor- and, if there are no nasty side effects, turn it on to avoid all this redundant resolution. 2. Alternatively, if turning on this cache is not deemed safe, we would need a way to tell the CMDocumentManager that the system id passed in was already resolved. We could use another property for this. You may have noticed that I have not really tackled the main complaint: the base location -> IFile resolution for large workspaces. This could be solved in a similar way by using a currently unused URIResolver feature: passing in options. If we do have it, we could pass in the base location IFile as an option. In this specific scenario though it seems to me that we don't have the IFile handy. The only thing available is a string location. And I further think that in this case the base location is a moot variable because the only thing needed seems to be the CM document for XHTML which is coming from the catalog. So the code could as well be passing in null for the base location. And finally, why is the XHTML CM document not being cached between files? I hope all this makes some sense. Looking forward to reading your comments before I try to create a fix; there may be things I'm overlooking because I lack historical background on this piece of code. I'd probably go for workaround #2, but in the long run I think any resolution responsibility should be taken out of the CMDocumentManagerImpl. It shouldn't have to know that there are multiple ways to get to the same CMDocument. Any chance we could shift to the base location being a workspace IPath instead, so that getting the IFile becomes a trivial matter? Perhaps provide a org.eclipse.wst.common.uriresolver.internal.ExtensibleURIResolver.resolve(IPath, String, String) method so callers can skip the expensive stuff? Created attachment 57945 [details]
Proposed short term fix
This is my proposed short term fix. It implements workaround #2 I described in my rather lengthy previous post. The fix enhances the CM document manager with a new property used to instruct it to resolve (or not) the URI passed in (the system ID). By default this property is set to true to preserve backward compatibility for other clients.The XHTML association provider sets this property to false to avoid the extra resolution. I hope the new property definition (the static final defined in CMDocumentManager can be considered as a non-breaking API change.
I did some measurements on a very large workspace (tens of projects) with auto-build turned off and only the HTML validator enabled. The performance improvements I observed are on the order of 3 times as fast for validating one project with many JSP/HTML files.
I would really appreciate it if this fix gets a good review as well as independent confirmation of the performance boost. For the long term I will open a number of bugs to re-design this area of URI resolution and global CM caching.
Requesting PMC review and approval. Please see comment #12 for an detailed description of the fix. I've reviewed this change and it looks safe and sound. Valentin can you open a bug to track the issue of reworking the XHTML resolver for WTP 2.0 (and add a link to that bug from here)? This reads like a good bug to fix in 1.5.3, but I would like a little more time to study it, and have some other people review it, and test against some "fringe" cases (e.g. "custom" XHMTL in JSP's, etc.). While that delay will miss this weeks M-build, if it proves worthy, I think it'll be good to get in next week's build. Reviewed patch and agree the code looks fine. Provided you get David's approval on review or additional test then I'll give this a +1 as well (as Arthur's PMC delegate). -1 I'm think it'd be better to wait on this code. Chances are, it's fine, but, I do think there's a small chance of odd regressions that would be hard to test and/or track down. The property approach is hard to make thread safe, and I'm not sure that class is never accessed by multiple threads. A totally separate "CMDocumentManagerNoResolver" class that essentially set the property before returning would be more thread safe. Plus, this is notoriously fragile code, so I'd prefer to put in a fix early in a release cycle ... M5 might be ok ... and get a bit better exposure. (Then if seemed to prove ok, we might decide to backport it to any future maintenance release). Not to mention, similar to what Craig mentioned, we really need to have a better "big picture" to feel safe with changes in this code, at least for me. Be sure to fix this along with bug 139092, just in case there's some interaction. Thanks very much for tackling this tough area, and my comments are in no way critical ... I probably just have a bit higher fear for any potential instability than others. Since this fix did not get PMC approval for 1.5.3, resetting the target to 2.0. Will try to commit the fix for RC0. Created attachment 64195 [details]
Patch for 2.0
2.0 patch committed to HEAD. Released for 2.0 builds > v200704181252 verified Closing as verified. |
While reviewing some profiling data, it was found that XMLCatalogIdResolver.resolve() takes up a significant amount of time to resolve URIs. Looking at the implementation, a few things stood out. The Common URI Resolver was called with every request to resolve() and that involves invoking all of the relevant extensions and also computing an IFile for the base resource. From what I can recall, the XMLCatalogIdResolver class was created exclusively to look up values in the catalogs, and if that's right, the IFile of the resource hardly matters. Following along that reasoning, the XMLCatalogIdResolver should really just call the XMLCatalogURIResolverExtension directly. Going further down that road (and reading the implementation of XMLCatalogURIResolverExtension), the IFile and base location parameters are unused and thus don't need to be found or corrected. Rewriting the resolve() method to this ends up being faster during operations that call it repeatedly, like validation of a project: public String resolve(String base, String publicId, String systemId) { String result = new XMLCatalogURIResolverExtension().resolve(null, base, publicId, systemId); return result } Of course, someone more familiar with the class needs to review this before any changes can be made.