Bug 296487 - Investigate using the PDE state for our workspace baseline
Summary: Investigate using the PDE state for our workspace baseline
Status: VERIFIED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: API Tools (show other bugs)
Version: 3.6   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 3.6 M4   Edit
Assignee: Michael Rennie CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance
Depends on:
Blocks: 233643
  Show dependency tree
 
Reported: 2009-11-30 10:57 EST by Michael Rennie CLA
Modified: 2009-12-18 14:40 EST (History)
2 users (show)

See Also:
darin.eclipse: review+
Olivier_Thomann: review+


Attachments
fix (228.80 KB, patch)
2009-11-30 11:00 EST, Michael Rennie CLA
no flags Details | Diff
proposed fix (3.44 KB, patch)
2009-12-01 13:59 EST, Michael Rennie CLA
no flags Details | Diff
update (5.99 KB, patch)
2009-12-01 14:36 EST, Michael Rennie CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Rennie CLA 2009-11-30 10:57:44 EST
Currently API tools creates and manages its own OSGi state for our workspace baseline. This effort is basically cloning the work PDE already does. We should investigate using the PDE state for our workspace baseline to avoid all the state management work altogether. 

We can also reap benefits from: 

1. not having to include the entire target platform in the workspace baseline
2. we can better compute the complete dependency trees for bundles in the workspace baseline
Comment 1 Michael Rennie CLA 2009-11-30 11:00:55 EST
Created attachment 153357 [details]
fix

Patch completely removes API tools handling any state in our workspace baseline. Also has delayed initialization for API component handles.
Comment 2 Michael Rennie CLA 2009-11-30 11:04:15 EST
Applied patch to HEAD, all tests + smoke tests pass. 

Please verify Olivier and Darin
Comment 3 Michael Rennie CLA 2009-11-30 11:11:55 EST
Some average timings (over 4 performance test runs on my Linux box)

******  Before Patch  ************

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.FullSourceBuildTests#testFullBuild()'
  Elapsed Process:        7.97s     

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.FullSourceBuildTests#testCleanFullBuild()'
  Elapsed Process:       11.86s     

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.ApiDescriptionTests#testCleanVisit()'
  Elapsed Process:        8.33s     

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.IncrementalBuildTests#testIncrementalBuildAll()'
  Elapsed Process:         972ms    

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.EnumIncrementalBuildTests#testIncremantalEnum()'
  Elapsed Process:         880ms    

****  After patch  *****

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.FullSourceBuildTests#testFullBuild()'
  Elapsed Process:        8.21s 
  
Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.FullSourceBuildTests#testCleanFullBuild()'
  Elapsed Process:       11.73s  
 
Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.ApiDescriptionTests#testCleanVisit()'
  Elapsed Process:        8.29s  

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.IncrementalBuildTests#testIncrementalBuildAll()'
  Elapsed Process:         340ms  
 
Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.EnumIncrementalBuildTests#testIncremantalEnum()'
  Elapsed Process:         280ms  

Scenario 'org.eclipse.pde.api.tools.builder.tests.performance.AnnotationIncrementalBuildTests#testIncrementalAnnot()'
  Elapsed Process:         262ms
Comment 4 Darin Wright CLA 2009-12-01 12:41:55 EST
Re-opening. For some yet unknown reason, this is corrupting PDE state when adding/removing a project in the workspace.
Comment 5 Michael Rennie CLA 2009-12-01 13:50:39 EST
The problem is coming from our code to try and look up a bundle in the OSGi state. We collect the bundle symbolic name from the manifest directly and pass that in to State.getBundle(name, version), which fails if a the bundle symbolic name has any post-amble - for example com.ibm.icu; singletone:=true;

We should be using ManifestElement.parseHeader(..) to get the symbolic name from the manifest and not collecting it directly by manifest.get(..).
Comment 6 Michael Rennie CLA 2009-12-01 13:59:51 EST
Created attachment 153511 [details]
proposed fix

Proposed fix, which uses ManifestHeader.parseHeader(..) and cleans up a small component handle leak. 

Running tests and smoke tests...
Comment 7 Michael Rennie CLA 2009-12-01 14:36:55 EST
Created attachment 153516 [details]
update

verifying test suite runs I found a problem with one of our tests, where were disposing a lone API component. The workspace baseline should be disposed during tearDown(), we should not be toasting a component by itself.
Comment 8 Michael Rennie CLA 2009-12-01 14:38:57 EST
all tests and smoke tests pass, released to HEAD. Please verify Darin W and Olivier
Comment 9 Michael Rennie CLA 2009-12-01 15:19:15 EST
(In reply to comment #8)
> all tests and smoke tests pass, released to HEAD. Please verify Darin W and
> Olivier

Target platform smoke tests reveled we were still setting properties for a bundle into the PDE state on a callback for initializing the workspace baseline - good catch Darin. Fix has been applied to not do this for the workspace baseline.
Comment 10 Olivier Thomann CLA 2009-12-10 12:11:41 EST
Looks good to me.
Comment 11 Darin Wright CLA 2009-12-14 11:45:49 EST
Verified.
Comment 12 Darin Wright CLA 2009-12-18 14:40:19 EST
Verified.