George Stephanos gaf.stephanos@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.
On Fri, Sep 27, 2013 at 11:13 AM, Alexandre Julliard julliard@winehq.orgwrote:
George Stephanos gaf.stephanos@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@winehq.org
George Stephanos gaf.stephanos@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.
On Fri, Oct 25, 2013 at 10:31 AM, Alexandre Julliard julliard@winehq.orgwrote:
George Stephanos gaf.stephanos@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@winehq.org
George Stephanos gaf.stephanos@gmail.com writes:
On Fri, Oct 25, 2013 at 10:31 AM, Alexandre Julliard julliard@winehq.org wrote:
George Stephanos <gaf.stephanos@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 -
George Stephanos