Rewrite project event forwarder. #1544
Conversation
bjorkstromm
requested review from
DustinCampbell,
david-driscoll,
filipw,
rchande and
akshita31
- The previous implementation had one lock for all events. This implementation only locks for same event. - The previous implementation had a 500ms delay before getting project information request. Don't think that's necessary anymore. Especially not after removing support for project.json. MSBuild project system will make sure all projects in the queue are processed before returning project information.
bjorkstromm
force-pushed
the
rewrite-project-event-forwarder
branch
from
e5a49b9 to
5ed2fd3
Compare
|
@filipw is there a way to know that a project system is enabled but not yet initialized? I think it would be wise in startup to wait for the project system to initialize before pushing project information. |
|
yeah at the moment, no. the |
|
Would these initialisation flags by any chance help with #1521? |
|
@nickspoons yes, |
| } | ||
|
|
||
| if (e != null) | ||
| var added = false; | ||
| using (await _eventLocks.GetOrAdd(workspaceEvent, _ => { added = true; return new AsyncLock(); }).LockAsync()) |
There was a problem hiding this comment.
I think what this code is trying to say is:
- For a given event type, attempt to add a lock over that workspace type
- If my valueCreator is called, I am the one who added the lock
- Because of that I can lock my lock and do my processing
However, per the remarks here:
However, the valueFactory delegate is called outside the locks to avoid the problems that can arise from executing unknown code under a lock. Therefore, GetOrAdd is not atomic with regards to all other operations on the ConcurrentDictionary<TKey,TValue> class. Since a key/value can be inserted by another thread while valueFactory is generating a value, you cannot trust that just because valueFactory executed, its produced value will be inserted into the dictionary and returned.
So I don't think this is going to have the desired effect.
There was a problem hiding this comment.
That's where something like BlockingCollection might work better in this scenario, we're just locking because HashSet is not thread safe, but in reality we want the data to push out asap, the delay doesn't need to be there for sure.
OnWorkspaceChanged adds an item to the queue synchronously. Then we spin up a dedicated thread that just sits and does TryTake(out var item, TimeSpan.Infinity) and just writes out to the emitter. This lets us queue these operations up, and dequeue them as quickly as possible.
Another option is to use System.Reactive and buffer the requests so that we pipe out all events every <500 ms (or whatever)>.
The blocking collection works well for LSP, so it should probably work okay here.
|
@rchande you are absolutely correct! Back to double locking (or do we even need locks?). Problem is that project system might be in a completely different state, so that was probably why it was double locking before, and it had a 500ms delay. The 500ms delay should not be needed as getting project info from MSBuild project system empties the processing queue before responding. The lock would then just assure we don’t send multiple events of the same form within a specific time. What are these events used for on VSCode side? |
|
@mholo65 C# extension sends these events in response to file change notifications from VS Code. I think we just need to make sure whatever we come up with maintains the same "correctness" we had before. IMO, it might be nice to make some more concrete statements about the gains than "Quickly tested, and it felt like large code bases loaded faster." :) |
|
what is the status of this? 😀 |
Probably not going to happen in current form. Without any tests around "how it should work", we'll probably just end up breaking things 😄 I must however say that the |
|
@mholo65 I will close this PR then, because they start to pile up - please open a new one when you feel like it's ready 😀 |
Quickly tested, and it felt like large code bases loaded faster.
Here be dragons...