Bugzilla – Full Text Bug Listing
| Summary: | Regressions from bug 432826: slow setTaskName on ProgressbarDialog on Linux and even worse on OSX | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Dani Megert <daniel_megert> |
| Component: | UI | Assignee: | Brian de Alwis <bsd> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | bsd, daniel.rolka, daniel_megert, emoffatt, pwebster |
| Version: | 4.4 | Flags: | pwebster:
review+
daniel_megert: review+ emoffatt: review+ daniel.rolka: review+ |
| Target Milestone: | 4.4 RC4 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | 432826, 436328 | ||
| Bug Blocks: | |||
|
Description
Dani Megert
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. |