Community
Participate
Working Groups
Build: I20090922-0800 The incremental API analysis builder performs poorly when there are changes to a bundle manifest. In my workspace doing an empty change to a bundle manifest (add/remove space, save), takes 1 minute on the first run, and a consistent 40 seconds on a subsequent run. It appears to be doing detailed API analysis on every project in my workspace, even those unrelated to the change. This is extremely painful when doing work that requires manifest changes (in my case editing manifest headers completely unrelated to API). It should be able to determine there is no work to do here - only certain headers have impact on API (BSN, version, import/export package, require bundle, classpath, execution environment come to mind). Note, I know there are existing bugs about poor API builder performance, but wanted to enter this very specific one about manifest file changes in particular. I will attach a debug trace showing builder invocations for multiple "no-op" changes to the bundle manifest of a single bundle (org.eclipse.equinox.p2.core).
Created attachment 147933 [details] Builder execution trace
The problem is that there is no API to know what kind of changes have been made to the manifest. So even if the change is a no-op change, we still perform a build.
The builder would need to remember a model of what the manifest looked like at the time of the previous build (at least the parts of it that affect API), and then compare the new state to this stored builder state to see if there are any interesting changes.
I think we must have the relevant state in our API components - i.e. exported packages, bundle version, etc. We should be able to tell if anything changed.
I had to do some changes in the manifest of 'org.eclipse.team.core' on which many of my source projects depend on. This lets Eclipse build for several minutes during which time user operations are very slow or even blocked.
Just happened again: My workspace showed be the blocking dialog for 10 minutes!
I'll take this one. From what I have found so far we have the no-op change problem for the manifest *and* for build.properties - we check the delta for entries for both and if either of them appear we do a full build. In the case of a manifest change, I think we only want to cause a full build of the project if one of the following headers changes value: Bundle-SymbolicName, Bundle-Version, Require-Bundle, Export-Package, Bundle-RequiredExecutionEnvironment, Import-Package or Bundle-ClassPath In the build.properties case we only want to cause a full build of the project if the jars.extra.classpath changes. I also found it interesting that we cascade the full build to all projects our builder returns as interesting. For example say we have projects A, B, C and D, where A's manifest is changed. When we build A we ask for the deltas for the collection of interesting projects [A, B, C, D] and we get the manifest change in A, so we fully build it. When we build B, we get the deltas and find the manifest change from A and fully build B - not because we do any special processing to determine if B needs to be fully built because of the manifest change in A, but simply because a manifest change appeared at all. I think this is the real problem, as we end up forcing full builds on the entire dependency graph simply because a manifest (or build.properties) change was found.
Did the fix now go into the N-build as discussed yesterday?
(In reply to comment #8) > Did the fix now go into the N-build as discussed yesterday? No, I opt'ed to try and finish up the delta processing to properly detect a no-op edit to the manifest or build.properties
Created attachment 192118 [details] proposed fix The patch prevents cascading the full build to the entire graph and has support for computing a simple set of changes for a manifest file and a build.properties file. Computing a no-op change requires a build state, so the first time a given project is built with the fix the project will be fully built (if a change was made to the manifest or build.properties) since there will be no saved state for its manifest / build.properties. All of the automated tests pass for me on Linux / Win7 64bit and my smoke testing so far has not broken the proposed fix.
Created attachment 192125 [details] update Heres an updated patch that parses and does a deep compare of the manifest attributes that we care about rather than just a string compare like in the other patch. Can't decide if this is better to have, as it makes detecting a no-op edit take longer than the other patch.
Created attachment 192136 [details] tiny update One more tiny update to sort header values before comparing them.
I committed the last patch so that we can test it in the N build tomorrow.
Bravo! I didn't do any specific correctness tests of the fix but the performance improvement is terrific: it goes down from 10m50s to below 1m!
Is there more work planned or should we close this?
(In reply to comment #15) > Is there more work planned or should we close this? OOPS: just got the following exception caused by recent change in ApiAnalysisBuilder.compareBuildProperties(...) !ENTRY org.eclipse.core.resources 4 2 2011-04-01 14:05:25.300 !MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.core.resources". !STACK 0 java.lang.ClassCastException: java.lang.String cannot be cast to java.util.Map$Entry at org.eclipse.pde.api.tools.internal.builder.ApiAnalysisBuilder.compareBuildProperties(ApiAnalysisBuilder.java:603) at org.eclipse.pde.api.tools.internal.builder.ApiAnalysisBuilder.shouldFullBuild(ApiAnalysisBuilder.java:497) at org.eclipse.pde.api.tools.internal.builder.ApiAnalysisBuilder.build(ApiAnalysisBuilder.java:355) at org.eclipse.core.internal.events.BuildManager$2.run(BuildManager.java:717) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:191) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:228) at org.eclipse.core.internal.events.BuildManager$1.run(BuildManager.java:281) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.core.internal.events.BuildManager.basicBuild(BuildManager.java:284) at org.eclipse.core.internal.events.BuildManager.basicBuildLoop(BuildManager.java:340) at org.eclipse.core.internal.events.BuildManager.build(BuildManager.java:363) at org.eclipse.core.internal.events.AutoBuildJob.doBuild(AutoBuildJob.java:143) at org.eclipse.core.internal.events.AutoBuildJob.run(AutoBuildJob.java:241) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Fixed the class cast in HEAD. Marking fixed, Curtis and Dani, can you please verify?
(In reply to comment #17) > Fixed the class cast in HEAD. > > Marking fixed, Curtis and Dani, can you please verify? The CCE is gone and the performance still good.
+1 Code changes look very reasonable and it definitely cuts down on the builder work.
