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.

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

Fix issues from dictionary caching #1353

Merged
merged 5 commits into from
Sep 11, 2023
Merged

Fix issues from dictionary caching #1353

merged 5 commits into from
Sep 11, 2023

Conversation

manodasanW
Copy link
Member

  • 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
9 checks passed
@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
2 participants
@manodasanW @jlaanstra