Issue1091

[martinmcclure]

To improve performance for large repositories issue 1091. Most of the time was being spent in js-signals code dealing with listener priorities (which Ungit does not use) and preventing duplicates (which Ungit does not appear to need). I've created an unprioritized version of js-signals, js-unprioritized-signals. This PR makes Ungit depend use js-unprioritized-signals rather than js-signals.

[jung-kim]

Brilliant! How is the speed now? is it at least functional?

Do we even need risky optimization that I've made?

[jung-kim]

I may clean up and push for #1277 but abandon the effort to only receive git log diffs from the server as that assumption changes have so much implications

[martinmcclure]

@jung-kim With this PR, Ungit is usable on a large repository, but still sluggish. Very rough profiling seems to indicate that a significant amount of time is being taken running Git commands. I'm interested in further speed improvements, but won't have much time to work on this for at least several days.

[jung-kim]

I understand and will definitely look for other improvements as well

I've done more thinking and did take a look at the changes in unprioritized-signals . I do agree with the spirit and intent of the changes. however I do have some few concerns:

1. we are now allowing duplicate listeners. as you have mentioned it shouldn't be a big problem but I can definitely see that when we navigate back and forth between repos it will be building up.
2. this library is super old and quite frankly I'm bit surprised that it is not deprecated. I'm beginning to suspect that there are other hidden problems with it that may come up later.

I think due to this, maybe we should replacing this library with micro-signals? It's implementation is dead simple and uses Set[] for the listeners so uniqueness is guaranteed.

Only thing is that it is a typescript...
Can we import typescript lib to node? sorry I'm bit weak in this and maybe we should find different plain js signal lib.

Do you think this makes sense?

[campersau]

@jung-kim here is the same "fix" with micro-signals master...campersau:micro-signals but with both of them I had memory problems in my testing see #1091 (comment) for other proposals.

[martinmcclure]

@jung-kim Thanks for the discussion, and for finding problems. I hadn't tried switching from repository to repository; when I do I do see that the listeners build up, with duplicates included. Before, I think the listeners would build up to the sum of all the repositories ever viewed. This seems odd and wasteful -- why would we need thousands of listeners for a repository that we're not even displaying? My (possibly naive) expectation is that we'd just drop the Signal instance when changing repositories.
I'll take a look at the discussion in #1091

[jung-kim]

Before, I think the listeners would build up to the sum of all the repositories ever viewed. This seems odd and wasteful -- why would we need thousands of listeners for a repository that we're not even displaying?

Absolutely agree, I think this is an oversight or perhaps a regression. There is no need for this.

My (possibly naive) expectation is that we'd just drop the Signal instance when changing repositories.

Doing clean up would work. But forgive me as I'm not sure if I'm ready to give up on more simpler and slicker replacement for this library as shown by @campersau . I will do my best to make myself focus on this issue and push out a solution soon.

@campersau thanks, and will see what the memory problem is

[martinmcclure]

@jung-kim I agree completely -- deeper/better solutions in the directions suggested by @campersau are preferable. I'm going to close this PR now in expectation that better approaches will be available soon.