Community
Participate
Working Groups
Build Identifier: I20110613-1736 The recent performance patch for the DeltaProcessor code resets the resolved classpath _only_ if it encounters a jar that contains chained jars in the resource delta. This can miss two types of state change: 1) An archive that had chained jars is updated to contain no chained jars. In this case the chained jars should get removed from the resolved classpath, if they are not pulled in by other jars. 2) setRawClasspath() is called with library jars that do not yet exist, so they are added to the invalid archive cache. The missing jars subsequently get built, triggering the delta processor code. Since the invalid archive cache only gets cleared when removing per-project info or when resetting the resolved classpath, the newly appearing jars remain in the invalid archive list and any classes within them remain unresolved. For #1, to determine if a previously chained jar entry should be removed from the resolved classpath you need to either keep a reference count for each jar on the classpath, or just reset the resolved classpath and recalculate it. Resetting the resolved classpath once per delta processor invocation doesn't appear to be a performance problem and is the simplest solution, so I will attach an initial patch that does that. For #2 it seems like ClasspathEntry.validateClasspathEntry() should always clear the invalid archive list, but JavaModelManager.resetClasspathListCache() resets both that list and the non-chaining jars list, so attach a second patch that adds a new JavaModelManager.resetInvalidArchiveListCache() function and calls it in the beginning of ClasspathEntry.validateClasspathEntry(). Strictly speaking, patch #1 will fix both issues when dealing with invocations of the delta processor code, but not resetting the invalid archive list from Classpath.validateClasspathEntry() seems like a potential bug, given that that function attempts to update that state. Also, I'd be happy to add a regression test for this issue if you point me at the appropriate place to add it. Reproducible: Always Steps to Reproduce: 1. Call setRawClasspath() with a library archive jar that doesn't exist 2. Validate that class files contained in the missing jar cause compilation erros/missing imports 3. Copy the missing jar into place, and refresh the project to pick it up 4. After the delta processor code executes, the new jar should resolve the missing classes, but it doesn't because the jar still exists in the invalid archive cache
Created attachment 203194 [details] Patch to DeltaProcessor.java Always reset the resolved classpath on any change to an archive jar, not just when it contains a chained jar.
Created attachment 203195 [details] Add a ResetInvalidArchivesCache() function Add a ResetInvalidArchivesCache() function and invoke it from ClasspathEntry.validateClasspathEntry()
Jay, please follow up.
(In reply to comment #0) > Also, I'd be happy to add a regression test for this issue if you point me at > the appropriate place to add it. > The tests usually go to JavaElementDeltaTests or ClasspathTests, which you can find in the project org.eclipse.jdt.core.tests.model. Olivier, this is indeed a regression from the bug 354332. But I think we could still keep the fixes we made already. Perhaps, should we reopen the bug 354332 and make it depend on this one?
(In reply to comment #4) > Olivier, this is indeed a regression from the bug 354332. But I think we could > still keep the fixes we made already. Perhaps, should we reopen the bug 354332 > and make it depend on this one? Yes, bug 354332 should be reopened to add the dependency on this one. If you think we should keep the fix in, then close it after this is done and work on a fix for this one. Now do you think we should request a rebuild for M2 or we can have M2 with this issue in ?
(In reply to comment #5) > Now do you think we should request a rebuild for M2 or we can have M2 with this > issue in ? Considering that what is failing is not a common use case and that we have workaround, I don't think we need an M2 rebuild.
This should be fixed before M3.
(In reply to comment #0) > Also, I'd be happy to add a regression test for this issue if you point me at > the appropriate place to add it. Any luck with the test? The patch looks good and it would be nice to have a regression test.
I have just dug my way out from under my end-of-quarter tasks. I'll take a look at adding tests tomorrow.
Created attachment 204894 [details] Patch to fix the DeltaProcessor regeression and fix validateClasspathEntry(), with tests
(In reply to comment #10) > Created attachment 204894 [details] > Patch to fix the DeltaProcessor regeression and fix validateClasspathEntry(), > with tests Patch looks good and thanks. But one of the new tests is failing when the entire ClasspathTests is run. On debug I found that the delta processor sees the lib2.jar as already existing. I would suggest we use bug specific names in the test, such as Bug357425, lib357425_a.jar, lib357425_b.jar etc. to make the tests pass. I could do this, but I can't alter the patch.
Created attachment 205212 [details] Update to name the test jar files uniquely I renamed that test as well to testBug357425() and moved it to the bottom of the file.
(In reply to comment #12) > Created attachment 205212 [details] > Update to name the test jar files uniquely > > I renamed that test as well to testBug357425() and moved it to the bottom of > the file. This patch passes all the tests. Jay, is anything else pending for this bug ? If not can you close it ? Please remember to set the iplog and review flags appropriately and make sure the copy right mentions the contribution.
(In reply to comment #13) > This patch passes all the tests. Jay, is anything else pending for this > bug ? If not can you close it ? Please remember to set the iplog and review > flags appropriately and make sure the copy right mentions the contribution. Thanks, Srikanth. I will release the fix soon.
Created attachment 205507 [details] Patch with copyrights Git patch, same as last one, but with updated copyright information.
Released in HEAD for 3.8 M3 with commit 30fef0e89bd163d63b2dda342f5f4be753476e00 Terry, once again, thanks for the contribution.
I filed bug 361922 for possible improvements on the test coverage side.
Verified for 3.8 M3 using build N20111022-2000
*** Bug 375249 has been marked as a duplicate of this bug. ***
(In reply to comment #6) > (In reply to comment #5) > > Now do you think we should request a rebuild for M2 or we can have M2 with this > > issue in ? > > Considering that what is failing is not a common use case and that we have > workaround, I don't think we need an M2 rebuild. Jay, 1. Any chance to backport the fix to version 3.7? (This bug is related to 3.8.2, but the bug still appears in 3.7.2, see Bug 375249, which you marked as duplicate.) 2. You mentioned a workaround, but I couldn't quite make out what you meant. Could you please elaborate on the topic? (The workarounds I desrcribed in Bug 375249 are nor stable nor close to comfortable.) Thanks for the reply!
(In reply to comment #20) > 1. Any chance to backport the fix to version 3.7? (This bug is related to > 3.8.2, but the bug still appears in 3.7.2, see Bug 375249, which you marked as > duplicate.) For reasons Satyam mentioned in bug 375249, comment #12, bug 375249 is probably not a duplicate of this one. Could you confirm if any of the 3.8 versions addresses your concern? > 2. You mentioned a workaround, but I couldn't quite make out what you meant. > Could you please elaborate on the topic? (The workarounds I desrcribed in Bug > 375249 are nor stable nor close to comfortable.) The workaround is same as the one you mentioned in bug 375249, comment #3. Sorry I didn't realize it wasn't clearly mentioned in this bug.
(In reply to comment #21) > The workaround is same as the one you mentioned in bug 375249, comment #3. > Sorry I didn't realize it wasn't clearly mentioned in this bug. To elaborate, try these steps: 1. Close the project in question. 2. Delete the invalidArchivesCache 3. Restart the workspace and reopen the project It's possible that the restarting the workspace could be avoided, but we would want to rule out other causes. Before reopening the workspace, ensure that the JAR in question is a valid archive.
