Skip to content

Fix issues from dictionary caching #1353

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

Jump to bottom
Merged
manodasanW merged 5 commits into from
Sep 11, 2023
Merged

Fix issues from dictionary caching #1353

manodasanW merged 5 commits into from
Sep 11, 2023

Conversation

@manodasanW
Copy link
Member

@manodasanW manodasanW commented Sep 11, 2023

  • Currently the caching that was put in place to improve performance is not safe from concurrent lookups due to using Dictionary.
  • Replacing it to ConcurrentDictionary is actually an API surface / assembly version change and it is unknown if the same perf improvements will still hold.
  • The caching was done based on the ABI type being IntPtr but it is possible for it to be not as demonstrated by the new tests that were added.
  • Some of the upcoming generics changes for AOT will probably improve perf, so this PR undoes the caching changes and we will re-evaluate the dictionary perf and whether a variant of this improvement is needed once we have the updated AOT generic changes.

Fixes #1295

@jlaanstra jlaanstra mentioned this pull request Sep 11, 2023
Closed
@manodasanW manodasanW merged commit 5721184 into master Sep 11, 2023
@manodasanW manodasanW deleted the manodasanw/fixdictionaryimpl branch September 11, 2023 21:43
manodasanW added a commit that referenced this pull request Sep 26, 2023
@manodasanW @jlaanstra
Update staging with latest (#1361)
414d2c2 
* Fix issues from dictionary caching (#1353)

* Add tests to exercise different ABI types for generic of dictionary

* Revert lookup cache to fix rests where ABI Signature can be different.

* Bring over API compat validation updates

* Undo comment

* Undo one more comment

* Simplify factory generated code. (#1355)

* Include CsWInRT.exe in the inputs so MSBuild reruns the target if CsWinRT.exe got updated. (#1356)

* Manually generate signatures for some PInvokes to handle SetLastError. (#1354)

* Manually generate signatures to handle SetLastError.

* Use new signatures on .NET 6 and newer.

* Use sbyte*

---------

Co-authored-by: Manodasan Wignarajah <mawign@microsoft.com>

---------

Co-authored-by: Johan Laanstra <jlaans@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jlaanstra jlaanstra jlaanstra approved these changes

@vineeththomasalex vineeththomasalex Awaiting requested review from vineeththomasalex

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multiple concurrent dictionary lookups via a dictionary wrapped in a ReadOnlyDictionary are causing the concurrent exception.

3 participants

@manodasanW @jlaanstra