Skip to content

Revert "Revert "Dynamic generic dictionary expansion feature (#32270) - #33343

Merged
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:gendict-fix
Mar 9, 2020
Merged

Revert "Revert "Dynamic generic dictionary expansion feature (#32270)#33343
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:gendict-fix

Conversation

@jkotas

@jkotasjkotas commented Mar 8, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#32828

@Dotnet-GitSync-Bot

Dotnet-GitSync-Bot commented Mar 8, 2020

Copy link
Copy Markdown
Collaborator

I couldn't add an area label to this PR.

Checkout this page to find out which area owner to ping, or please add exactly one area label to help train me in the future.

@jkotas
jkotas requested a review from fadimounirMarch 8, 2020 03:40
jkotas
jkotas commented Mar 8, 2020
View reviewed changes
Comment threadsrc/coreclr/src/vm/genericdict.cpp Outdated
}
CONTRACT_END;

Dictionary* pDictionary = pMD->GetMethodDictionary();

@jkotasjkotasMar 8, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is where the race condition was. We have fetched the Dictionary, then some other thread expanded the dictionary, and then we fetched the number of slot from the new Dictionary; but still kept using the old Dictionary.

@fadimounirfadimounirMar 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Jan for tracking this down!

@jkotas

jkotas commented Mar 8, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

@fadimounir I have done detailed review of the code, and I believe that I have found the race condition. See the comment above. I have also tightened up the implementation in a few places as I was doing it.

@jkotas
jkotas merged commit fc2e56c into dotnet:masterMar 9, 2020
@jkotas
jkotas deleted the gendict-fix branch March 9, 2020 21:40
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@fadimounirfadimounirfadimounir approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-VM-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Assert failure HasPerInstInfo() methodtable.h Line: 2908

3 participants

@jkotas@Dotnet-GitSync-Bot@fadimounir