C#: Populate labels earlier #6012
Conversation
hvitved
force-pushed
the
csharp/early-labels
branch
from
c852b49 to
6b63e03
Compare
| @@ -52,6 +54,10 @@ protected void DefineLabel(IEntity entity) | |||
| finally | |||
| { | |||
| writingLabel = false; | |||
| if (labelQueue.Any()) | |||
There was a problem hiding this comment.
Should this be while loop rather than an if so that we write out all the labels from the queue?
There was a problem hiding this comment.
DefineLabel is recursive, so a while loop is not needed.
There was a problem hiding this comment.
This recursion might result in strange behavior: the first item (entity A) in the queue is dequeued, and if in the meantime DefineLabel is being called on another entity, then entity A might be enqueued again, which means it goes to the end of the queue, so the queue doesn't give any guarantee on the ordering.
There was a problem hiding this comment.
The order is not super important, what matters is that we populate the labels as early as possible. For example, for a method like
IEnumerable<int> M(string s) { }we will (simplified) generate the following TRAP:
#0 = {#1} M({#2} s)
#1 = {#3}<{#4}>
#2 = "string"
#3 = "IEnumerable<>"
#4 = "int"
so we are still using labels before their definition, but the definition will follow shortly after, as opposed to the old implementation.
| protected void DefineLabel(IEntity entity) | ||
| { | ||
| if (writingLabel) | ||
| { | ||
| // Don't define a label whilst writing a label. | ||
| PopulateLater(() => DefineLabel(entity)); | ||
| labelQueue.Enqueue(entity); |
There was a problem hiding this comment.
Is there a guarantee that labelQueue gets emptied? It seems possible that there are items left in the queue:
Thread 1 starts defining label on entity A (writingLabel == true), Thread 2 enters if (writingLabel), Thread 1 finishes all the work even the finally block, and then Thread 2 enqueues the entity.
There was a problem hiding this comment.
It's all single threaded: Multi-threading is only for extracting different files in parallel.
There was a problem hiding this comment.
Hmm, I should have known this. 🤦
Fixes github/codeql-action#544.
https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/1128/
This improves TRAP import time significantly on some projects:
