Skip to content

C#: Populate labels earlier #6012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hvitved merged 1 commit into from
Jun 15, 2021
Merged

C#: Populate labels earlier #6012

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion csharp/extractor/Semmle.Extraction/Context.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ public class Context
// A recursion guard against writing to the trap file whilst writing an id to the trap file.
private bool writingLabel = false;

private readonly Queue<IEntity> labelQueue = new();

protected void DefineLabel(IEntity entity)
{
if (writingLabel)
{
// Don't define a label whilst writing a label.
PopulateLater(() => DefineLabel(entity));
labelQueue.Enqueue(entity);
Copy link
Contributor

@tamasvajk tamasvajk Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hvitved hvitved Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all single threaded: Multi-threading is only for extracting different files in parallel.

Copy link
Contributor

@tamasvajk tamasvajk Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I should have known this. 🤦

}
else
{
Expand All @@ -52,6 +54,10 @@ protected void DefineLabel(IEntity entity)
finally
{
writingLabel = false;
if (labelQueue.Any())
Copy link
Contributor

@edoardopirovano edoardopirovano Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be while loop rather than an if so that we write out all the labels from the queue?

Copy link
Contributor Author

@hvitved hvitved Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DefineLabel is recursive, so a while loop is not needed.

Copy link
Contributor

@tamasvajk tamasvajk Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hvitved hvitved Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
DefineLabel(labelQueue.Dequeue());
}
}
}
}
Expand Down