Skip to content

Do not use any locking - #1384

Closed
mattleibow wants to merge 9 commits into
mainfrom
dev/fix-1383
Closed

Do not use any locking#1384
mattleibow wants to merge 9 commits into
mainfrom
dev/fix-1383

Conversation

@mattleibow

Copy link
Copy Markdown
Contributor

Description of Change

It appears that using locks triggers a paint...

So, we can't do that. I have pulled in the code from https://github.com/VSadov/NonBlocking to test out. This will actually be collapsed into the minimum requirements for an IntPtr dictionary.

Bugs Fixed

None.

Behavioral Changes

None.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Updated documentation

I have pulled in the code from https://github.com/VSadov/NonBlocking to test out. This will actually be collapsed into the minimum requirements for an IntPtr dictionary.
@mattleibow
mattleibow marked this pull request as draft July 6, 2020 00:52
Comment thread binding/Binding/HandleDictionary.cs Outdated
Comment on lines +167 to +168
// TODO: remove from the dictionary
// or, better yet, do this in a single op

@mattleibow mattleibow Jul 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the reason I originally switched away from the dictionary. There is no easy way to have an atomic comparison between the key and value. I only want to remove if the key AND value match.

The reason for this is just because the native side will have created a brand new object at the same location, and we mustn't remove it form the dictionary. We only do that if this is our handle AND our object.

To work around this for now, I do an UPDATE, and replace the value with a dummy object. This means that the dictionary always grows. Maybe with the new dictionary type, this update operation could be a TryRemove(key, value) and it checks both before removing.


// volatile read to make sure we read the element before we read the _newTable
// that would guarantee that as long as _newTable == null, entryValue cannot be forwarded.
var entryValue = Volatile.Read(ref entry.value);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could check the value here, and then jump out if it didn't match.

@mattleibow

Copy link
Copy Markdown
Contributor Author

From @kekekeks in gitter:

Why are you using AddOrUpdate?
I think that TryAdd is better
since the thread that has lost the race will just destroy its newly created object and take one from the dictionary (edited)
it's way less bug prone to have a guarantee that a particular handle is only getting added to the table once

@mattleibow

Copy link
Copy Markdown
Contributor Author

from @kekekeks

@mattleibow I think we could use an alternative approach

  1. implement some lock abstraction that would use EnterCriticalSection on win32 and Monitor.Enter otherwise
  2. copy-paste ConcurrentDictionarty from dotnet/runtime and make it use said lock abstraction
    might be costly due to additional P/Invoke though

Base automatically changed from master to main February 5, 2021 08:30
@mattleibow mattleibow added this to the v2.88.1 milestone May 22, 2022
@mattleibow

Copy link
Copy Markdown
Contributor Author

This is probably not needed after #1817

Either way, this is not the right approach.

@mattleibow mattleibow closed this Aug 12, 2022
@mattleibow
mattleibow deleted the dev/fix-1383 branch August 12, 2022 12:03
@mattleibow mattleibow removed this from the v2.88.1 milestone Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[BUG] SkiaSharp can call WM_PAINT handler and random COM callbacks when creating or obtaning objects

1 participant