[PATCH v2 0/1] MR7668: wbemprox: Copy table in exec_select_view() instead of using per-table CS.
-- v2: wbemprox: Copy table in exec_select_view() instead of using per-table CS.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/wbemprox/builtin.c | 7 +-- dlls/wbemprox/class.c | 12 ++--- dlls/wbemprox/query.c | 31 ++++++----- dlls/wbemprox/services.c | 2 +- dlls/wbemprox/table.c | 93 ++++++++++++++++++++------------ dlls/wbemprox/wbemprox_private.h | 14 ++--- 6 files changed, 91 insertions(+), 68 deletions(-)
diff --git a/dlls/wbemprox/builtin.c b/dlls/wbemprox/builtin.c index 646d93e3978..968d18061b3 100644 --- a/dlls/wbemprox/builtin.c +++ b/dlls/wbemprox/builtin.c @@ -4637,12 +4637,7 @@ void init_table_list( void ) { list_init( &tables[ns] ); for (i = 0; i < builtin_namespaces[ns].table_count; i++) - { - struct table *table = &builtin_namespaces[ns].tables[i]; - InitializeCriticalSectionEx( &table->cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); - table->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": table.cs" ); - list_add_tail( &tables[ns], &table->entry ); - } + list_add_tail( &tables[ns], &builtin_namespaces[ns].tables[i].entry ); table_list[ns] = &tables[ns]; } } diff --git a/dlls/wbemprox/class.c b/dlls/wbemprox/class.c index 8e585c2f360..53822372bb7 100644 --- a/dlls/wbemprox/class.c +++ b/dlls/wbemprox/class.c @@ -114,7 +114,7 @@ static HRESULT WINAPI enum_class_object_Next( { struct enum_class_object *ec = impl_from_IEnumWbemClassObject( iface ); struct view *view = ec->query->view; - struct table *table; + const struct table *table; static int once = 0; HRESULT hr; ULONG i, j; @@ -219,7 +219,7 @@ HRESULT EnumWbemClassObject_create( struct query *query, LPVOID *ppObj ) return S_OK; }
-static struct record *create_record( struct table *table ) +static struct record *create_record( const struct table *table ) { UINT i; struct record *record; @@ -536,7 +536,7 @@ static HRESULT WINAPI class_object_Next( struct class_object *obj = impl_from_IWbemClassObject( iface ); struct enum_class_object *iter = impl_from_IEnumWbemClassObject( obj->iter ); struct view *view = iter->query->view; - struct table *table = get_view_table( view, obj->index ); + const struct table *table = get_view_table( view, obj->index ); BSTR prop; HRESULT hr; UINT i; @@ -637,7 +637,7 @@ static BSTR get_body_text( const struct table *table, UINT row, UINT *len ) static BSTR get_object_text( const struct view *view, UINT index ) { UINT len, len_body, row = view->result[index]; - struct table *table = get_view_table( view, index ); + const struct table *table = get_view_table( view, index ); BSTR ret, body;
len = ARRAY_SIZE( L"\ninstance of %s\n{%s\n};" ); @@ -686,7 +686,7 @@ static HRESULT WINAPI class_object_SpawnInstance( { struct class_object *co = impl_from_IWbemClassObject( iface ); struct enum_class_object *ec = impl_from_IEnumWbemClassObject( co->iter ); - struct table *table = get_view_table( ec->query->view, co->index ); + const struct table *table = get_view_table( ec->query->view, co->index ); IEnumWbemClassObject *iter; struct record *record; HRESULT hr; @@ -888,7 +888,7 @@ static HRESULT WINAPI class_object_GetMethod( { struct class_object *co = impl_from_IWbemClassObject( iface ); IWbemClassObject *in, *out; - struct table *table; + const struct table *table; unsigned int i; HRESULT hr;
diff --git a/dlls/wbemprox/query.c b/dlls/wbemprox/query.c index a6599334f58..ef975990f0e 100644 --- a/dlls/wbemprox/query.c +++ b/dlls/wbemprox/query.c @@ -29,9 +29,9 @@
WINE_DEFAULT_DEBUG_CHANNEL(wbemprox);
-static HRESULT append_table( struct view *view, struct table *table ) +static HRESULT append_table( struct view *view, const struct table *table ) { - struct table **tmp; + const struct table **tmp; if (!(tmp = realloc( view->table, (view->table_count + 1) * sizeof(*tmp) ))) return E_OUTOFMEMORY; view->table = tmp; view->table[view->table_count++] = table; @@ -54,7 +54,7 @@ HRESULT create_view( enum view_type type, enum wbm_namespace ns, const WCHAR *pa
case VIEW_TYPE_SELECT: { - struct table *table = find_table( ns, class ); + const struct table *table = find_table( ns, class ); HRESULT hr;
if (table && (hr = append_table( view, table )) != S_OK) @@ -610,7 +610,7 @@ static HRESULT do_query( enum wbm_namespace ns, const WCHAR *str, struct query * }
static HRESULT get_antecedent_table( enum wbm_namespace ns, const WCHAR *assocclass, const WCHAR *dependent, - struct table **table ) + const struct table **table ) { BSTR antecedent = NULL; struct path *path = NULL; @@ -657,7 +657,7 @@ static HRESULT exec_assoc_view( struct view *view ) { ULONG count; IWbemClassObject *obj; - struct table *table; + const struct table *table; VARIANT var;
IEnumWbemClassObject_Next( iter, WBEM_INFINITE, 1, &obj, &count ); @@ -697,15 +697,20 @@ static HRESULT exec_select_view( struct view *view ) { UINT i, j = 0, len; enum fill_status status = FILL_STATUS_UNFILTERED; - struct table *table; + const struct table *table;
if (!view->table_count) return S_OK;
table = view->table[0]; if (table->fill) { - clear_table( table ); - status = table->fill( table, view->cond ); + struct table *table_copy; + + if (!(table_copy = get_table_writeable_copy( table ))) return E_OUTOFMEMORY; + clear_table( table_copy ); + status = table_copy->fill( table_copy, view->cond ); + release_table( table ); + table = view->table[0] = table_copy; } if (status == FILL_STATUS_FAILED) return WBEM_E_FAILED; if (!table->num_rows) return S_OK; @@ -869,7 +874,7 @@ static UINT count_key_columns( const struct table *table ) static BSTR build_relpath( const struct view *view, UINT table_index, UINT result_index, const WCHAR *name ) { BSTR class, proplist, ret = NULL; - struct table *table = view->table[table_index]; + const struct table *table = view->table[table_index]; UINT row = view->result[result_index]; UINT num_keys, len;
@@ -1164,7 +1169,7 @@ static HRESULT map_view_index( const struct view *view, UINT index, UINT *table_ return S_OK; }
-struct table *get_view_table( const struct view *view, UINT index ) +const struct table *get_view_table( const struct view *view, UINT index ) { switch (view->type) { @@ -1185,7 +1190,7 @@ HRESULT get_propval( const struct view *view, UINT index, const WCHAR *name, VAR { HRESULT hr; UINT column, row, table_index, result_index; - struct table *table; + const struct table *table; VARTYPE vartype; void *val_ptr = NULL; LONGLONG val; @@ -1382,7 +1387,7 @@ HRESULT put_propval( const struct view *view, UINT index, const WCHAR *name, VAR { HRESULT hr; UINT row, column, table_index, result_index; - struct table *table; + const struct table *table; LONGLONG val;
if ((hr = map_view_index( view, index, &table_index, &result_index )) != S_OK) return hr; @@ -1412,7 +1417,7 @@ HRESULT get_properties( const struct view *view, UINT index, LONG flags, SAFEARR SAFEARRAY *sa; BSTR str; UINT i, table_index, result_index, count = 0; - struct table *table; + const struct table *table; HRESULT hr; LONG j = 0;
diff --git a/dlls/wbemprox/services.c b/dlls/wbemprox/services.c index 3b55a534de9..29d2d576614 100644 --- a/dlls/wbemprox/services.c +++ b/dlls/wbemprox/services.c @@ -886,7 +886,7 @@ static HRESULT WINAPI wbem_services_ExecMethod( struct path *path; WCHAR *str; class_method *func; - struct table *table; + const struct table *table; HRESULT hr;
TRACE( "%p, %s, %s, %#lx, %p, %p, %p, %p\n", iface, debugstr_w(strObjectPath), diff --git a/dlls/wbemprox/table.c b/dlls/wbemprox/table.c index 37aa546ad2b..c7b2911ad82 100644 --- a/dlls/wbemprox/table.c +++ b/dlls/wbemprox/table.c @@ -346,59 +346,52 @@ void free_table( struct table *table ) free( (WCHAR *)table->name ); free_columns( (struct column *)table->columns, table->num_cols ); free( table->data ); - - table->cs.DebugInfo->Spare[0] = 0; - DeleteCriticalSection( &table->cs ); free( table ); }
-void release_table( struct table *table ) +void release_table( const struct table *ctable ) { - if (!--table->refs) - { - clear_table( table ); - if (table->flags & TABLE_FLAG_DYNAMIC) - { - EnterCriticalSection( &table_list_cs ); - list_remove( &table->entry ); - table->removed = TRUE; - LeaveCriticalSection( &table_list_cs ); + struct table *table = (struct table *)ctable;
- LeaveCriticalSection( &table->cs ); - free_table( table ); - return; - } - } - LeaveCriticalSection( &table->cs ); + EnterCriticalSection( &table_list_cs ); + if (InterlockedDecrement( &table->refs )) goto done; + clear_table( table ); + if (!(table->flags & TABLE_FLAG_DYNAMIC)) goto done; + list_remove( &table->entry ); + table->removed = TRUE; + free_table( table ); +done: + LeaveCriticalSection( &table_list_cs ); }
-struct table *grab_table( struct table *table ) +const struct table *grab_table( const struct table *table ) { - EnterCriticalSection( &table->cs ); - if (table->removed) - { - LeaveCriticalSection( &table->cs ); - return NULL; - } - table->refs++; + EnterCriticalSection( &table_list_cs ); + if (table->removed) table = NULL; + else InterlockedIncrement( (LONG *)&table->refs ); + LeaveCriticalSection( &table_list_cs ); return table; }
-struct table *find_table( enum wbm_namespace ns, const WCHAR *name ) +const struct table *find_table( enum wbm_namespace ns, const WCHAR *name ) { + const struct table *ret = NULL; struct table *table;
if (ns == WBEMPROX_NAMESPACE_LAST) return NULL;
+ EnterCriticalSection( &table_list_cs ); LIST_FOR_EACH_ENTRY( table, table_list[ns], struct table, entry ) { if (name && !wcsicmp( table->name, name )) { - TRACE("returning %p\n", table); - return grab_table( table ); + ret = grab_table( table ); + break; } } - return NULL; + LeaveCriticalSection( &table_list_cs ); + TRACE("returning %p\n", table); + return ret; }
struct table *create_table( const WCHAR *name, UINT num_cols, const struct column *columns, @@ -419,11 +412,41 @@ struct table *create_table( const WCHAR *name, UINT num_cols, const struct colum table->refs = 0; table->removed = FALSE; list_init( &table->entry ); - InitializeCriticalSectionEx( &table->cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); - table->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": table.cs"); return table; }
+struct table *get_table_writeable_copy( const struct table *src ) +{ + struct table *table; + BYTE *data = NULL; + ULONG_PTR size; + unsigned int i; + + if (src->flags & TABLE_FLAG_DYNAMIC) + { + TRACE( "Table %p is dynamic.\n", src ); + return (struct table *)grab_table( src ); + } + + size = 0; + for (i = 0; i < src->num_cols; ++i) + size += get_column_size( src, i ); + size *= src->num_rows_allocated; + + if (size && src->data) + { + if (!(data = malloc( size ))) return NULL; + memcpy( data, src->data, size ); + } + if (!(table = create_table( src->name, src->num_cols, src->columns, src->num_rows, src->num_rows_allocated, + data, src->fill ))) + { + free( data ); + return NULL; + } + return (struct table *)grab_table( table ); +} + BOOL add_table( enum wbm_namespace ns, struct table *table ) { struct table *iter; @@ -449,7 +472,7 @@ BOOL add_table( enum wbm_namespace ns, struct table *table )
BSTR get_method_name( enum wbm_namespace ns, const WCHAR *class, UINT index ) { - struct table *table; + const struct table *table; UINT i, count = 0; BSTR ret;
@@ -474,7 +497,7 @@ BSTR get_method_name( enum wbm_namespace ns, const WCHAR *class, UINT index )
WCHAR *get_first_key_property( enum wbm_namespace ns, const WCHAR *class ) { - struct table *table; + const struct table *table; WCHAR *ret = NULL; UINT i;
diff --git a/dlls/wbemprox/wbemprox_private.h b/dlls/wbemprox/wbemprox_private.h index 5e585049aa6..164ba53f3a5 100644 --- a/dlls/wbemprox/wbemprox_private.h +++ b/dlls/wbemprox/wbemprox_private.h @@ -121,7 +121,6 @@ struct table UINT flags; struct list entry; LONG refs; - CRITICAL_SECTION cs; BOOL removed; };
@@ -154,7 +153,7 @@ struct record { UINT count; struct field *fields; - struct table *table; + const struct table *table; };
struct keyword @@ -179,7 +178,7 @@ struct view const struct property *proplist; /* SELECT query */ const struct expr *cond; UINT table_count; - struct table **table; + const struct table **table; UINT result_count; UINT *result; }; @@ -214,12 +213,13 @@ HRESULT create_view( enum view_type, enum wbm_namespace, const WCHAR *, const st const struct property *, const struct expr *, struct view ** ); void destroy_view( struct view * ); HRESULT execute_view( struct view * ); -struct table *get_view_table( const struct view *, UINT ); +const struct table *get_view_table( const struct view *, UINT ); void init_table_list( void ); enum wbm_namespace get_namespace_from_string( const WCHAR *namespace ); -struct table *find_table( enum wbm_namespace, const WCHAR * ); -struct table *grab_table( struct table * ); -void release_table( struct table * ); +const struct table *find_table( enum wbm_namespace, const WCHAR * ); +const struct table *grab_table( const struct table * ); +struct table *get_table_writeable_copy( const struct table * ); +void release_table( const struct table * ); struct table *create_table( const WCHAR *, UINT, const struct column *, UINT, UINT, BYTE *, enum fill_status (*)(struct table *, const struct expr *) ); BOOL add_table( enum wbm_namespace, struct table * );
v2: - fix potential races in table list manipulation.
This is winehq bug 56859. I had been thinking about fixing this but in a different way. We shouldn't be holding a reference to the table (via query->view) from the enumerator object. We can instead create a list of class objects and copy data from the table view using the record structure. That also simplifies the class object implementation.
That probably means 'struct record' should stop referencing query->view? record->view->table is now also used class_object, not only enum_class_object.
Are you already working on that? If not I could probably try to do something between the lines of that.
The table reference in struct record is only used to get the column index, which can be stored instead of the table reference.
I had started on this and I can take it from here if you want.
Yes, sure. Closing this.
This merge request was closed by Paul Gofman.
With that redesign are you planning to still get rid of those per table lock? Even if those locks won't be held outside of wbemprox method calls, I am afraid that still can deadlock on, e. g., parallel associated queries which access multiple tables.
Right, the ASSOCIATORS OF queries are problematic. We'd get rid of the table lock for the enumerator but there might still be more than one result table. I have a patch that instanciates tables from static data and gets rid of the per table lock.
-
Hans Leidekker (＠hans) -
Paul Gofman
-
Paul Gofman (＠gofman)