Skip to content
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.

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

Improved threadsafety for FileIndexTable.FileToIndex #18137

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

KevinRansom
Copy link
Member

@KevinRansom KevinRansom commented Dec 13, 2024

This is a slight improvement to the threadsafety of the FileIndexTable.FileToIndex property.

The issue with the original code was when two threads were racing to add a file, if each thread has the same value for filePath and it hadn't already been added, the code would queue behind the lock and add the file twice with different ids.

The original fix to the code was to grab a lock whenever it was likely to add an index, this nicely synchronized the query whether the item was already in the dictionary.

This fix goes back to the original smaller lock just around the specific code path that adds the item. Before adding an item, it rechecks within the lock that the item is in the list. If it isn't it adds it. Otherwise it bails.

Note:
fileToIndexTable is a ConcurrentDictionary<string, int> therefore threadsafe and efficient when accessing.
we synchronize access on the fileToIndexTable syncblock.

/cc @T-Gro , @vzarytovskii , @Martin521

@KevinRansom KevinRansom requested a review from a team as a code owner December 13, 2024 07:58
Copy link
Contributor

github-actions bot commented Dec 13, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

Copy link
Contributor

@Martin521 Martin521 left a comment

Choose a reason for hiding this comment

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

Yes, that makes sense. The write operation for the non-normalized entry does not need to be in the lock, it cannot create a duplicate entry.

@majocha
Copy link
Contributor

majocha commented Dec 13, 2024

Another approach to do this would be to use a lazy valueFactory with ConcurrentDictionary.GetOrAdd to ensure single execution.

@KevinRansom KevinRansom added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Dec 13, 2024
@KevinRansom KevinRansom merged commit 471fdc8 into dotnet:main Dec 13, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants