Bug 564708 - Migrate template resolver setup job to declarative services component
Summary: Migrate template resolver setup job to declarative services component
Status: RESOLVED FIXED
Alias: None
Product: EGit
Classification: Technology
Component: UI (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 5.11   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance
Depends on:
Blocks:
 
Reported: 2020-06-27 19:04 EDT by Alex Blewitt CLA
Modified: 2021-02-15 05:39 EST (History)
2 users (show)

See Also:


Attachments
Stack traces (78.75 KB, text/plain)
2021-02-12 11:13 EST, Thomas Wolf CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Blewitt CLA 2020-06-27 19:04:23 EDT
The Activator.registerTemplateVariableResolvers method starts up a job to register the templates with the Java plugin. We can move this to a DS component instead and have that be used to register the content, and so save on the Activator time of EGit UI.
Comment 1 Thomas Wolf CLA 2021-02-12 02:56:03 EST
Somehow I had overlooked that we had a bug for this. The EGit UI Activator has been cleaned up using OSGi DS in the change series with tip at https://git.eclipse.org/r/c/egit/egit/+/176147 .
Comment 2 Lars Vogel CLA 2021-02-12 03:09:46 EST
Thanks for working on this Thomas. Looking forward to have EGit removed from the startup time as we see it in https://bugs.eclipse.org/bugs/show_bug.cgi?id=569285#c2.
Comment 3 Thomas Wolf CLA 2021-02-12 03:16:35 EST
Well... the work still is done, just at other times. The main effect of this will be that EGit UI will disappear from your Activator time "watchlist". Most of the work done formerly by the Activator still is done exactly the same way as before. (Only the configuration check has been moved from a Display.asyncExec() into a true background job.)

One also has to be careful: doing lots of things on component activation or in response to an APP_STARTUP_COMPLETED event is just as bad as doing lots of things in an Activator.

The main effect of these changes is to shift work from one place in time to a few other places, so it may _appear_ that startup is swifter now. (Which is what counts in the end, I guess.)
Comment 4 Alex Blewitt CLA 2021-02-12 08:17:12 EST
Yes, doing things later is important :)

The observation is that when you load a class or resource from a bundle, it blocks until the bundle's activator has run. So even something as simple as looking up the decorator icon for a Git project will block until all the steps in the Activator has finished executing.

Moving it to a DS component means that accessing a class/icon from a bundle is no longer blocked by all the setup jobs completing. (In the case of some of the jobs in the EGit package, sometimes it's loading the closure of required classes that can be a longer delay than actually running the job.)

Finally if we move it out of the startup and into components, there's a chance (but no guarantee) that they'll be activated on background threads. When Eclipse (and indeed, EGit) first started, multi-core systems were still new; these days, everyone has a much higher number of (virtual) cores that we can utilise more effectively at start-up.

So the effect - moving it off the activator - is highly desirable, even if the sum of total work over all cores remains the same :)
Comment 5 Thomas Wolf CLA 2021-02-12 11:13:41 EST
Created attachment 285542 [details]
Stack traces

I have to revert making the debug listener of EGit core a DS component: https://git.eclipse.org/r/c/egit/egit/+/176203 .

That one causes Eclipse to crash on startup, and not come up at all. Reverting that one commit works; having the EGit UI debug listener as a DS component doesn't seem to cause trouble.

Strangely I didn't see that in tests when starting a child Eclipse. But it crashed my development Eclipse when I installed EGit nightly there. Had to do the revert on the command-line (and edit two files with a normal text editor). Yuck!

Stack traces in the attachment.
Comment 6 Thomas Wolf CLA 2021-02-12 12:27:22 EST
(In reply to Thomas Wolf from comment #5)
> I have to revert making the debug listener of EGit core a DS component:
> https://git.eclipse.org/r/c/egit/egit/+/176203 .

Reverted. Seems that before the EGit core debug listener could be made a DS, the EGit core Activator would need to be stripped.
Comment 7 Alex Blewitt CLA 2021-02-12 13:00:46 EST
So I think what's happening here is that there's a trigger caused by the DS component loading a class from the EGit bundle, which is triggering the bundle's activator, which is then deadlocking (probably on a synchronized method?) because there's two threads involved; the DS starter and the main equinox launcher.

You could probably defer the DS startup by using the same waiting operation that you do for the other approach but you're right, it's a case of getting rid of the activator in total before you can win.

I'll pick up the jobs approach I put together and see if I can help reduce the work that the startup is doing which will help.
Comment 8 Thomas Wolf CLA 2021-02-12 14:15:51 EST
(In reply to Alex Blewitt from comment #7)
> So I think what's happening here is that there's a trigger caused by the DS
> component loading a class from the EGit bundle, which is triggering the
> bundle's activator, which is then deadlocking (probably on a synchronized
> method?) because there's two threads involved; the DS starter and the main
> equinox launcher.
> 
> You could probably defer the DS startup by using the same waiting operation
> that you do for the other approach but you're right, it's a case of getting
> rid of the activator in total before you can win.
> 
> I'll pick up the jobs approach I put together and see if I can help reduce
> the work that the startup is doing which will help.

I didn't see a deadlock. I rather have the impression that the framework wants to instantiate the debug listener very early. That instantiation attempt causes egit.core.Activator.start() to be executed. That currently does a lot, and some of these operations cause core.resources to get activated. But since it all happens much earlier than normal, the instance location isn't set yet, and we get exceptions.  

The problem with EGit core is that we can't just move stuff to jobs. Some of the things it does need to happen during start(). Some other things can perhaps be delayed, perhaps even until first use.
Comment 9 Alex Blewitt CLA 2021-02-12 14:24:58 EST
Yeah, DS is up and running very early in the splash screen process, and with no dependencies is immediately activated, so you'll see this.

If you have a dependency on (say) the IWorkspace being available, then it won't be activated until that point. That's why the other dependency (on the platform being available or the E4 start has been triggered) you'll see that happening.

I think in this case there's other things in the Activator which might require the other services be up and running. It's a chicken and egg scenario to try and get them all out of the way :)

I wrote something to allow registering an IResourceChangeListener published as a service which is auto-registered to the IWorkspace (rather than having the IWorkspace.add/remove change listener) but unfortunately EGit goes all the way back to a really old version of Eclipse so you can't take advantage of that yet.

More than happy to try and assist further with this and I'd like to help.
Comment 10 Thomas Wolf CLA 2021-02-12 14:41:04 EST
(In reply to Alex Blewitt from comment #9)
> Yeah, DS is up and running very early in the splash screen process, and with
> no dependencies is immediately activated, so you'll see this.
> 
> If you have a dependency on (say) the IWorkspace being available, then it
> won't be activated until that point. That's why the other dependency (on the
> platform being available or the E4 start has been triggered) you'll see that
> happening.
> 
> I think in this case there's other things in the Activator which might
> require the other services be up and running. It's a chicken and egg
> scenario to try and get them all out of the way :)
> 
> I wrote something to allow registering an IResourceChangeListener published
> as a service which is auto-registered to the IWorkspace (rather than having
> the IWorkspace.add/remove change listener) but unfortunately EGit goes all
> the way back to a really old version of Eclipse so you can't take advantage
> of that yet.
> 
> More than happy to try and assist further with this and I'd like to help.

I didn't know the workspace was registered as an IWorkspace service! That registration exists even in Neon.3; just the ResourceChangeListenerRegistrar exists only as of 4.17. So we might indeed declare a reference on IWorkspace in the debug options listener to delay EGit core activation until after cor.resources has been activated. I'll give that a try.
Comment 11 Thomas Wolf CLA 2021-02-12 15:45:11 EST
(In reply to Thomas Wolf from comment #10)
> I didn't know the workspace was registered as an IWorkspace service! That
> registration exists even in Neon.3; just the ResourceChangeListenerRegistrar
> exists only as of 4.17. So we might indeed declare a reference on IWorkspace
> in the debug options listener to delay EGit core activation until after
> cor.resources has been activated. I'll give that a try.

Yup, that would work. But it's a bit of a hack to add that dependency to the debug listener when that listener itself doesn't need that dependency just because the Activator needs it. But that gives us a way to move more things out of the EGit core activator and have them triggered once the workspace is there.
Comment 12 Lars Vogel CLA 2021-02-15 05:39:17 EST
With latest I-build and night update site, I see EGit core prominent in the activator startup list, but EGit UI has reduced, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=569285#c8

See https://www.vogella.com/tutorials/EclipsePerformance/article.html#example-tracing-the-startup-time-of-plug-ins for how to generate this yourself.