Re: [PATCH 1/2] advapi32: HKCR merge: foundation
George Stephanos <gaf.stephanos(a)gmail.com> writes:
@@ -224,13 +248,138 @@ static NTSTATUS open_key( HKEY *retkey, ACCESS_MASK access, OBJECT_ATTRIBUTES *a
}
+static LSTATUS create_hkcr_struct( HKEY *hkey, opened_hkcr_t **hkcr ) +{ + UINT_PTR handle = nb_hkcr_handles, i; + LSTATUS ret = ERROR_SUCCESS; + + EnterCriticalSection( &hkcr_handles_cs ); + + for (i = 0; i < nb_hkcr_handles; i++) + { + if (!hkcr_handles[i]) + { + handle = i; + break; + } + }
You need some sort of free list instead of a linear search. Also you can probably avoid one level of pointers and store objects directly. Returning a pointer to the object outside of the critical section is not a good idea.
+static HKEY resolve_hkcr( HKEY hkey ) +{ + HKEY ret; + UINT_PTR idx = (UINT_PTR)hkey >> 2; + opened_hkcr_t *hkcr; + if (idx <= nb_hkcr_handles) + { + EnterCriticalSection( &hkcr_handles_cs );
The count needs to be protected by the critical section too. Also please write a better subject line for the commit. -- Alexandre Julliard julliard(a)winehq.org
On Fri, Sep 27, 2013 at 11:13 AM, Alexandre Julliard <julliard(a)winehq.org>wrote:
George Stephanos <gaf.stephanos(a)gmail.com> writes:
@@ -224,13 +248,138 @@ static NTSTATUS open_key( HKEY *retkey, ACCESS_MASK access, OBJECT_ATTRIBUTES *a
}
+static LSTATUS create_hkcr_struct( HKEY *hkey, opened_hkcr_t **hkcr ) +{ + UINT_PTR handle = nb_hkcr_handles, i; + LSTATUS ret = ERROR_SUCCESS; + + EnterCriticalSection( &hkcr_handles_cs ); + + for (i = 0; i < nb_hkcr_handles; i++) + { + if (!hkcr_handles[i]) + { + handle = i; + break; + } + }
You need some sort of free list instead of a linear search.
Alright. Also you can probably avoid one level of pointers and store objects
directly.
I thought about this. If I store objects directly, accessing any would require a lock on the whole table so I guarantee it's not moved or reallocated elsewhere. This would obviously be pretty slow.
Returning a pointer to the object outside of the critical section is not a good idea.
The critical section just protects the table and not the handles/structs themselves.
+static HKEY resolve_hkcr( HKEY hkey ) +{ + HKEY ret; + UINT_PTR idx = (UINT_PTR)hkey >> 2; + opened_hkcr_t *hkcr; + if (idx <= nb_hkcr_handles) + { + EnterCriticalSection( &hkcr_handles_cs );
The count needs to be protected by the critical section too.
Also please write a better subject line for the commit.
-- Alexandre Julliard julliard(a)winehq.org
George Stephanos <gaf.stephanos(a)gmail.com> writes:
You need some sort of free list instead of a linear search.
Alright.
Also you can probably avoid one level of pointers and store objects directly.
I thought about this. If I store objects directly, accessing any would require a lock on the whole table so I guarantee it's not moved or reallocated elsewhere. This would obviously be pretty slow.
You already have a lock on the whole table anyway.
Returning a pointer to the object outside of the critical section is not a good idea.
The critical section just protects the table and not the handles/structs themselves.
Yes, that's the problem. The structs have to be protected too. -- Alexandre Julliard julliard(a)winehq.org
On Fri, Oct 25, 2013 at 10:31 AM, Alexandre Julliard <julliard(a)winehq.org>wrote:
George Stephanos <gaf.stephanos(a)gmail.com> writes:
You need some sort of free list instead of a linear search.
Alright.
Also you can probably avoid one level of pointers and store objects directly.
I thought about this. If I store objects directly, accessing any would require a lock on the whole table so I guarantee it's not moved or reallocated elsewhere. This would obviously be pretty slow.
You already have a lock on the whole table anyway.
I do. But it's only used when creating new handles or on table reallocations. Earlier I could reallocate the table without worrying about the handles. In your case that same lock would have to be grabbed when *accessing* any of the handles so I make sure the table is not being reallocated. That results in: no two handles can be accessed concurrently as opposed to my current code where they can. I might be missing something though :|
Returning a pointer to the object outside of the critical section is not a good idea.
The critical section just protects the table and not the handles/structs themselves.
Yes, that's the problem. The structs have to be protected too.
Maybe a lock inside each struct?
-- Alexandre Julliard julliard(a)winehq.org
George Stephanos <gaf.stephanos(a)gmail.com> writes:
On Fri, Oct 25, 2013 at 10:31 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
George Stephanos <gaf.stephanos(a)gmail.com> writes:
> You need some sort of free list instead of a linear search. > > Alright. > > Also you can probably avoid one level of pointers and store > objects directly. > > > I thought about this. If I store objects directly, accessing any would > require a lock on the whole table so I guarantee it's not moved or > reallocated elsewhere. This would obviously be pretty slow.
You already have a lock on the whole table anyway.
I do. But it's only used when creating new handles or on table reallocations. Earlier I could reallocate the table without worrying about the handles.
In your case that same lock would have to be grabbed when *accessing* any of the handles so I make sure the table is not being reallocated. That results in: no two handles can be accessed concurrently as opposed to my current code where they can. I might be missing something though :|
You still have to grab the lock on every access; I very much doubt that releasing it a little earlier is going to make a difference. Of course you can make things faster by making them non thread-safe, but that's not a good idea.
> Returning a pointer to the object outside of the critical section > is not > a good idea. > > The critical section just protects the table and not the > handles/structs themselves.
Yes, that's the problem. The structs have to be protected too.
Maybe a lock inside each struct?
You would need to demonstrate an real app that shows heavy contention on that lock to make it worth the trouble. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
George Stephanos