Community
Participate
Working Groups
The fix for bug 436328 introduced some regressions (see dependent bugs). We either need to fix them for RC4 or revert the fix for bug 436328. NOTE: The performance problem that bug 436328 fixed in AccumulatingProgressMonitor, is not new. That code did not change in the past 6 years (until the recent fix).
Patch pushed to: https://git.eclipse.org/r/27725 I can't reproduce the exception reported in bug 436356. Doing further testing.
(In reply to Brian de Alwis from comment #2) > I can't reproduce the exception reported in bug 436356. Doing further > testing. Oops, that's a mistake: I can consistently reproduce bug 436356; can't reproduce bug 436328.
(In reply to Brian de Alwis from comment #2) > Patch pushed to: > > https://git.eclipse.org/r/27725 How does adding the synchronization back affect the performance fix (in numbers)? > I can't reproduce the exception reported in bug 436356. Doing further > testing. Yes, it involves threading. Reproducible steps would have to use breakpoints at the right locations.
(In reply to Dani Megert from comment #4) > How does adding the synchronization back affect the performance fix (in > numbers)? It improves it, if you can believe it. Using Max's 'broken-progressbar' example from bug 432826 timing with System.currentTimeMills() and discarding the first run: Without synchronized: Total: 273 ms Total: 224 ms Total: 261 ms Total: 269 ms Total: 272 ms Total: 243 ms Total: 193 ms Average: 247ms With synchronized: Total: 188 ms Total: 237 ms Total: 255 ms Total: 193 ms Total: 254 ms Total: 192 ms Total: 170 ms Average: 212ms
More importantly, marking 'synchronized' does not reintroduce the performance issue originally reported in bug 432826.
(In reply to Dani Megert from comment #4) (In reply to Brian de Alwis from comment #6) Sounds good. This bug covers both regression. Are you also providing a fix for bug 436356?
(In reply to Dani Megert from comment #7) > Sounds good. This bug covers both regression. Are you also providing a fix > for bug 436356? This patch definitely fixes 436356; and from Laurent's investigation 436328 looks to have the same cause. So there's nothing further to do, to my knowledge.
(In reply to Brian de Alwis from comment #8) > (In reply to Dani Megert from comment #7) > > Sounds good. This bug covers both regression. Are you also providing a fix > > for bug 436356? > > This patch definitely fixes 436356; and from Laurent's investigation 436328 > looks to have the same cause. So there's nothing further to do, to my > knowledge. So, you confirm it fixes said bug too?
(In reply to Dani Megert from comment #9) > So, you confirm it fixes said bug too? I cannot confirm that this patch fixes bug 436328 as I have been able to reproduce 436328 locally. The bug occurs as part of a fairly lengthy workflow and since the bug is sporadic, it's hard to confirm anything with certainty. I have however found a different way to reliably trigger these NPEs from within EGit. I've been unable to trigger the exception with the patch. During my testing, I encountered similar sporadic NPEs with delete-project. I haven't been able to cause the delete-project exception either. So I'm pretty confident this fix addresses the issue and there should be no further surprises.
Component lead +1 PW
I hit a snag on further testing. Updated bug 436356 PW
(In reply to Paul Webster from comment #12) > I hit a snag on further testing. Updated bug 436356 > > PW That turned out to be a different and old issue.
OK, I've double checked. +1 (again) PW
+1 for the latest patch...
Released with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6bc8ff2f527619764f5aebffd87321d74a079635 PW
Verified in 4.4.0.I20140604-2000 using the approach outlined in bug 436328c6.
