Bug 389250 - Lots of stuff leaked when closing and reopening views
Summary: Lots of stuff leaked when closing and reopening views
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.2   Edit
Hardware: All All
: P3 major with 1 vote (vote)
Target Milestone: 4.2.2   Edit
Assignee: Oleg Besedin CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance
Depends on: 379581
Blocks: 385272
  Show dependency tree
 
Reported: 2012-09-11 08:59 EDT by Dani Megert CLA
Modified: 2013-01-15 09:05 EST (History)
7 users (show)

See Also:


Attachments
Shows the leaks (36.84 KB, application/zip)
2012-09-11 09:09 EDT, Dani Megert CLA
no flags Details
Null some toolbar fields in CTF (784 bytes, patch)
2012-09-25 15:44 EDT, Bogdan Gheorghe CLA
no flags Details | Diff
Context allocation trace (24.19 KB, image/png)
2012-10-08 07:26 EDT, Dani Megert CLA
no flags Details
Reference graph (2.05 KB, application/zip)
2012-10-08 08:09 EDT, Dani Megert CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2012-09-11 08:59:06 EDT
M20120909-2000.

Lots of objects are leaked in a simple close/open part scenario.

Simple test case:
1. prepare a perspective with only the Navigator view open
2. reset the instance counter of your profiler
3. close the Navigator
4. reopen the Navigator

==> see many instance being leaked, e.g. 3 ToolItem(s) and 2 ToolBar(s)
Comment 1 Dani Megert CLA 2012-09-11 09:09:00 EDT
Created attachment 220933 [details]
Shows the leaks

The leaks at the top (above HashMap / 24) should be ignored. Those instances grow (and get GCed) constantly even when nothing happens (bug 389251).
Comment 2 Oleg Besedin CLA 2012-09-19 10:32:16 EDT
The CommandContributionItem entries and ElementReference are probably the same issue as described in the bug 389335 .
Comment 3 Oleg Besedin CLA 2012-09-24 10:33:57 EDT
Added fix into R4_2_maintenance for HandledMenuItemImpl and ParameterizedCommand's:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=d9d9f209f7f9a8d8a0c08381ee8f8bf51279fa2c
Comment 4 Oleg Besedin CLA 2012-09-25 10:09:35 EDT
Fixed EclipseContext leak and its related structures in R4_2_maintenance:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=bba83be0f4032360e1d8eebad230ecc55240f36e
Comment 5 Oleg Besedin CLA 2012-09-25 15:20:44 EDT
The ToolBar leak might be related to CTabFolder#minMaxTb
Comment 6 Bogdan Gheorghe CLA 2012-09-25 15:44:46 EDT
Created attachment 221486 [details]
Null some toolbar fields in CTF

Oleg, here is a patch you can try.
Comment 7 Bogdan Gheorghe CLA 2012-09-25 15:57:47 EDT
Released the patch to master - we can try out tonight's nightly to see if it made a difference (I'm guessing no, but it doesn't hurt to null out the fields).
Comment 8 John Arthorne CLA 2012-09-26 09:34:43 EDT
Can this be marked FIXED or still in progress?
Comment 9 Oleg Besedin CLA 2012-09-26 10:05:33 EDT
(In reply to comment #8)
> Can this be marked FIXED or still in progress?

In progress.
Comment 10 Oleg Besedin CLA 2012-09-26 10:29:22 EDT
Fixed a Color object leaked by the splash screen:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=8f25fbc4d8ff121d8cbe86c2e1e931d12998b1fb

This leak is not a big deal, I am addressing it to isolate Color leaks associated with parts close / open.
Comment 11 Oleg Besedin CLA 2012-09-26 10:58:23 EDT
Fixes leaking image from the drop-down menu button in the part stack:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=e5657ae23fa549e6dd34b30723f9189317146c99
Comment 12 Oleg Besedin CLA 2012-09-26 16:29:21 EDT
(In reply to comment #11)
> Fixes leaking image from the drop-down menu button in the part stack:
> 
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?h=R4_2_maintenance&id=e5657ae23fa549e6dd34b30723f9189317146c99

Oops, that was wrong. Reverted.
Comment 13 Oleg Besedin CLA 2012-09-27 10:27:45 EDT
Fixing image leak from the "shadow" image on the CTabFolder:

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?h=R4_2_maintenance&id=33b09f270cc969502514aff47fb2125128ba5db1

I also removed caching of those images via Display.E4_SHADOW_IMAGE as it was not used, was incorrect (only one last image would have being accessible and disposed), and made code more complicated.
Comment 14 Karen Butzke CLA 2012-09-27 10:33:18 EDT
Oleg, did you just fix bug 379581?
Comment 15 Oleg Besedin CLA 2012-09-27 13:54:29 EDT
(In reply to comment #7)
> Released the patch to master - we can try out tonight's nightly to see if it
> made a difference (I'm guessing no, but it doesn't hurt to null out the
> fields).

Thanks Bogdan! Using N20120926-2000, the ToolBar and related SWT items are no longer show up in the "leaks" list.
Comment 16 Oleg Besedin CLA 2012-09-27 14:03:40 EDT
(In reply to comment #14)
> Oleg, did you just fix bug 379581?

Yes, indeed! Thank you!
Comment 17 Oleg Besedin CLA 2012-10-02 15:56:13 EDT
I think this bug mostly has being fixed.

In my tests only two items left:

(a) a little leak in the sash rendered (opened bug 390968 as it touches aspects that I am not familiar with). This is not significant and only appears when a last part in the sash is getting repeatedly closed and re-opened. 

(b) a possible leaks of a single EclipseContext in the open/close part cycle. This shows up in the profiler, but I can't pin it down. This might be a profiler or VM artifact, see below.

I spend a good deal of time on tracking down (b) and starting to wonder if it is real or not. 
- all the contexts we allocate in the scenario are getting properly disposed
- the profiler shows that the "leaking" context is an empty context with no parent
- I changed code removing all but one context that we allocate in the scenario. The one remaining context (PartRenderingEngine.safeCreateGui) was needed for UI to function, but it does not fit the "empty context" description above. The profiler still showed one leaking empty context.
- I tracked down all references that profiler shows to the suspicious context and they aren't getting changed in the scenario; hence they can't produce the leak
Comment 18 Dani Megert CLA 2012-10-08 07:26:02 EDT
> (b) a possible leaks of a single EclipseContext in the open/close part
> cycle. This shows up in the profiler, but I can't pin it down. This might be
> a profiler or VM artifact, see below.
> 
> I spend a good deal of time on tracking down (b) and starting to wonder if
> it is real or not. 
> - all the contexts we allocate in the scenario are getting properly disposed
> - the profiler shows that the "leaking" context is an empty context with no
> parent
> - I changed code removing all but one context that we allocate in the
> scenario. The one remaining context (PartRenderingEngine.safeCreateGui) was
> needed for UI to function, but it does not fit the "empty context"
> description above. The profiler still showed one leaking empty context.
> - I tracked down all references that profiler shows to the suspicious
> context and they aren't getting changed in the scenario; hence they can't
> produce the leak


In my profiler (OptimizeIt) it also leaks 1 EclipseContext on each open/close. The allocation trace (see attached picture) indicates that the object created in PartRenderingEngine.safeCreateGui() is note freed.
Comment 19 Dani Megert CLA 2012-10-08 07:26:12 EDT
.
Comment 20 Dani Megert CLA 2012-10-08 07:26:48 EDT
Created attachment 222014 [details]
Context allocation trace
Comment 21 Dani Megert CLA 2012-10-08 08:09:55 EDT
Created attachment 222019 [details]
Reference graph
Comment 22 Dani Megert CLA 2012-10-08 08:10:26 EDT
It looks like the contexts are disposed at some point but still referenced. See attached reference graph.
Comment 23 Dani Megert CLA 2012-10-08 08:28:49 EDT
org.eclipse.e4.core.internal.contexts.ValueComputation objects are also still leaked.
Comment 26 Oleg Besedin CLA 2012-10-18 09:48:13 EDT
I think the general situation with closing/opening parts is pretty good now (with the small issue left described in the bug 390968).
Comment 27 Oleg Besedin CLA 2012-10-18 09:48:29 EDT
.
Comment 28 Oleg Besedin CLA 2012-10-30 13:47:59 EDT
Verified using I20121030-0800 and M20121024-1600.
Comment 29 Dani Megert CLA 2013-01-15 09:05:02 EST
(In reply to comment #26)
> I think the general situation with closing/opening parts is pretty good now
> (with the small issue left described in the bug 390968).

Verified this in N20130114-2000: much better now!