Bug 63911 - ExtensionsParser should use serviceTracker
Summary: ExtensionsParser should use serviceTracker
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Incubator (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows 2000
: P3 normal (vote)
Target Milestone: 3.0 RC2   Edit
Assignee: Rafael Chaves CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance
Depends on:
Blocks:
 
Reported: 2004-05-25 12:57 EDT by Pascal Rapicault CLA
Modified: 2005-09-27 09:12 EDT (History)
2 users (show)

See Also:


Attachments
patches for runtime and tests.runtime (3.09 KB, application/octet-stream)
2004-06-10 11:46 EDT, Rafael Chaves CLA
no flags Details
patch for runtime (7.94 KB, patch)
2004-06-10 16:39 EDT, Rafael Chaves CLA
no flags Details | Diff
patch for runtime (4.38 KB, patch)
2004-06-10 17:37 EDT, Rafael Chaves CLA
no flags Details | Diff
patch for tests.runtime (1.94 KB, patch)
2004-06-10 17:38 EDT, Rafael Chaves CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pascal Rapicault CLA 2004-05-25 12:57:37 EDT
Here is a copy of a bug that has been entered against PluginParser but that is
also probably true for the ExtensionsParser.

The PluginConverterImpl and various Classes in core.runtime aquire a 
ServiceReference to the SAXParserFactory each time they want to parse an XML 
file.  After parsing each file we end up releasing the service reference and 
discarding the SAXParserFactory.

This likely only effects performance on initial startup when we are reading 
all the xml files the first time and caching the data (This will happen twice 
for each installed bundle).  While this operation is not extremely expensive, 
it does not come for free either.  It would help to use a ServiceTracker 
object that can be used to cache our reference to the SAXParserFactory service.
Comment 1 Jeff McAffer CLA 2004-06-09 22:05:20 EDT
take a quick look for RC2.  The idea is to make a service tracker and keep it 
around then just to getService() when you need the service.
Comment 2 Rafael Chaves CLA 2004-06-10 11:46:22 EDT
Created attachment 11883 [details]
patches for runtime and tests.runtime
Comment 3 Rafael Chaves CLA 2004-06-10 11:47:26 EDT
Tom, could you review the changes?
Comment 4 Thomas Watson CLA 2004-06-10 15:56:36 EDT
I suggest you don't initialize the ServiceTracker object in a static 
initializer.  This will prevent the runtime bundle from working if it is 
stopped and started again without refreshing (note that this is not really a 
supported scenario, but I would not recommend adding more reasons to why this 
cannot be done)

I suggest you add two static methods to ExtensionsParser:
  initiaize() - to initialize and open the ServiceTracker object
  shutdown() - to close and null out the ServiceTracker object

The initialize() should be called by InternalPlatform.start() and shutdown 
should be called by InternalPlatform.stop().
Comment 5 Rafael Chaves CLA 2004-06-10 16:39:07 EDT
Created attachment 11909 [details]
patch for runtime

(did not include the patch for tests.runtime since they don't have anything
interesting)
Comment 6 Rafael Chaves CLA 2004-06-10 16:40:02 EDT
Incorporated Tom's suggestions (with slight differences). Jeff, please review.
Comment 7 Rafael Chaves CLA 2004-06-10 17:37:46 EDT
Created attachment 11910 [details]
patch for runtime
Comment 8 Rafael Chaves CLA 2004-06-10 17:38:13 EDT
Created attachment 11911 [details]
patch for tests.runtime
Comment 9 Rafael Chaves CLA 2004-06-10 17:40:57 EDT
I had already released my previous changes when we realized that we had a thread
safety problem. These patches are against what is in HEAD. You may want to apply
the changes and then compare against two revisions before the latest one.

Tom, please review. Jeff will re-review and release the code tonight.
Comment 10 Rafael Chaves CLA 2004-06-10 17:43:31 EDT
Jeff, just to make sure you noticed it: the changes to tests.runtime must be
released at the same time.
Comment 11 Thomas Watson CLA 2004-06-10 18:18:24 EDT
patch looks good to me.
Comment 12 Jeff McAffer CLA 2004-06-10 22:35:03 EDT
patch for runtime and runtime.tests comitted.