[PATCH 0/1] MR11136: vbscript: Cache host DISPIDs resolved through named items.
Identifier lookups that resolve into a named item's host dispatch called GetIDsOfNames on the host for every access, while native vbscript resolves a name once and reuses the DISPID afterwards, observable with a host that counts the queries. Cache resolved ids on the named item and drop the cache together with the item's dispatch. A 300k-iteration loop reading a host member now queries the host once instead of 300000 times, matching native. With a host that scans a 1600-entry member table per query the loop drops from 35 s to 0.1 s; with a trivial host the time is unchanged. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11136
From: Francis De Brabandere <francisdb@gmail.com> Identifier lookups that resolve into a named item's host dispatch called GetIDsOfNames on the host for every access, while native vbscript resolves a name once and reuses the DISPID afterwards, observable with a host that counts the queries. Cache resolved ids on the named item and drop the cache together with the item's dispatch. A 300k-iteration loop reading a host member now queries the host once instead of 300000 times, matching native. With a host that scans a 1600-entry member table per query the loop drops from 35 s to 0.1 s; with a trivial host the time is unchanged. --- dlls/vbscript/interp.c | 4 +- dlls/vbscript/tests/vbscript.c | 90 ++++++++++++++++++++++++++++++++++ dlls/vbscript/vbscript.c | 51 +++++++++++++++++++ dlls/vbscript/vbscript.h | 5 ++ 4 files changed, 148 insertions(+), 2 deletions(-) diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c index 1b071d6f077..c92c3aff632 100644 --- a/dlls/vbscript/interp.c +++ b/dlls/vbscript/interp.c @@ -211,7 +211,7 @@ static HRESULT lookup_identifier(exec_ctx_t *ctx, BSTR name, vbdisp_invoke_type_ if(ctx->func->code_ctx->named_item && ctx->func->code_ctx->named_item->disp && !(ctx->func->code_ctx->named_item->flags & SCRIPTITEM_CODEONLY)) { - hres = disp_get_id(ctx->func->code_ctx->named_item->disp, name, invoke_type, TRUE, &id); + hres = named_item_get_disp_id(ctx->func->code_ctx->named_item, name, invoke_type, TRUE, &id); if(SUCCEEDED(hres)) { ref->type = REF_DISP; ref->u.d.disp = ctx->func->code_ctx->named_item->disp; @@ -242,7 +242,7 @@ static HRESULT lookup_identifier(exec_ctx_t *ctx, BSTR name, vbdisp_invoke_type_ LIST_FOR_EACH_ENTRY(item, &ctx->script->named_items, named_item_t, entry) { if((item->flags & SCRIPTITEM_GLOBALMEMBERS)) { - hres = disp_get_id(item->disp, name, invoke_type, FALSE, &id); + hres = named_item_get_disp_id(item, name, invoke_type, FALSE, &id); if(SUCCEEDED(hres)) { ref->type = REF_DISP; ref->u.d.disp = item->disp; diff --git a/dlls/vbscript/tests/vbscript.c b/dlls/vbscript/tests/vbscript.c index 6fe7893d3b0..9162435c7b8 100644 --- a/dlls/vbscript/tests/vbscript.c +++ b/dlls/vbscript/tests/vbscript.c @@ -304,15 +304,19 @@ static const IDispatchVtbl persistent_named_item_vtbl = { static IDispatch persistent_named_item = { &persistent_named_item_vtbl }; +static int shadow_method_dispid_queries, shadow_prop_dispid_queries; + static HRESULT WINAPI shadowing_GetIDsOfNames(IDispatch *iface, REFIID riid, LPOLESTR *names, UINT name_cnt, LCID lcid, DISPID *ids) { ok(name_cnt == 1, "name_cnt = %u\n", name_cnt); if(!wcscmp(names[0], L"shadowMethod")) { + shadow_method_dispid_queries++; *ids = 1; return S_OK; } if(!wcscmp(names[0], L"shadowProp")) { + shadow_prop_dispid_queries++; *ids = 2; return S_OK; } @@ -3080,6 +3084,91 @@ static void test_named_item_no_dim_routes_to_host(void) ok(!ref, "ref = %ld\n", ref); } +static void test_named_item_dispid_caching(void) +{ + IActiveScriptParse *parse; + IActiveScript *script; + HRESULT hres; + LONG ref; + + /* The engine resolves a host member name through GetIDsOfNames once and + * reuses the DISPID for subsequent lookups, within and across script + * chunks. */ + + script = create_vbscript(); + hres = IActiveScript_QueryInterface(script, &IID_IActiveScriptParse, (void**)&parse); + ok(hres == S_OK, "Could not get IActiveScriptParse: %08lx\n", hres); + + SET_EXPECT(GetLCID); + hres = IActiveScript_SetScriptSite(script, &ActiveScriptSite); + ok(hres == S_OK, "SetScriptSite failed: %08lx\n", hres); + CHECK_CALLED(GetLCID); + + hres = IActiveScript_AddNamedItem(script, L"shadowingItem", SCRIPTITEM_ISVISIBLE); + ok(hres == S_OK, "AddNamedItem failed: %08lx\n", hres); + + SET_EXPECT(OnStateChange_INITIALIZED); + hres = IActiveScriptParse_InitNew(parse); + ok(hres == S_OK, "InitNew failed: %08lx\n", hres); + CHECK_CALLED(OnStateChange_INITIALIZED); + + SET_EXPECT(OnStateChange_CONNECTED); + hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_CONNECTED); + ok(hres == S_OK, "SetScriptState failed: %08lx\n", hres); + CHECK_CALLED(OnStateChange_CONNECTED); + + shadow_method_dispid_queries = 0; + shadow_prop_dispid_queries = 0; + + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + SET_EXPECT(host_shadow_propput); + SET_EXPECT(host_shadow_propget); + SET_EXPECT(host_shadow_method); + hres = IActiveScriptParse_ParseScriptText(parse, L"shadowProp = 5\nx = shadowProp\nshadowMethod\n", + L"shadowingItem", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08lx\n", hres); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + CHECK_CALLED(host_shadow_propput); + CHECK_CALLED(host_shadow_propget); + CHECK_CALLED(host_shadow_method); + + ok(shadow_prop_dispid_queries == 1, "shadowProp resolved %d times, expected 1\n", shadow_prop_dispid_queries); + ok(shadow_method_dispid_queries == 1, "shadowMethod resolved %d times, expected 1\n", shadow_method_dispid_queries); + + /* a later script chunk reuses the cached ids too */ + SET_EXPECT(OnEnterScript); + SET_EXPECT(OnLeaveScript); + SET_EXPECT(host_shadow_propput); + SET_EXPECT(host_shadow_method); + hres = IActiveScriptParse_ParseScriptText(parse, L"shadowProp = 6\nshadowMethod\n", + L"shadowingItem", NULL, NULL, 0, 0, 0, NULL, NULL); + ok(hres == S_OK, "ParseScriptText failed: %08lx\n", hres); + CHECK_CALLED(OnEnterScript); + CHECK_CALLED(OnLeaveScript); + CHECK_CALLED(host_shadow_propput); + CHECK_CALLED(host_shadow_method); + + ok(shadow_prop_dispid_queries == 1, "shadowProp resolved %d times after second chunk, expected 1\n", + shadow_prop_dispid_queries); + ok(shadow_method_dispid_queries == 1, "shadowMethod resolved %d times after second chunk, expected 1\n", + shadow_method_dispid_queries); + + SET_EXPECT(OnStateChange_DISCONNECTED); + SET_EXPECT(OnStateChange_INITIALIZED); + SET_EXPECT(OnStateChange_CLOSED); + hres = IActiveScript_Close(script); + ok(hres == S_OK, "Close failed: %08lx\n", hres); + CHECK_CALLED(OnStateChange_DISCONNECTED); + CHECK_CALLED(OnStateChange_INITIALIZED); + CHECK_CALLED(OnStateChange_CLOSED); + + IActiveScriptParse_Release(parse); + ref = IActiveScript_Release(script); + ok(!ref, "ref = %ld\n", ref); +} + static void test_const_at_top_level(void) { IActiveScriptParse *parse; @@ -3521,6 +3610,7 @@ START_TEST(vbscript) test_named_item_sub_shadowing(); test_named_item_dim_shadowing(); test_named_item_no_dim_routes_to_host(); + test_named_item_dispid_caching(); test_const_at_top_level(); test_cross_parse_name_redef(); test_scriptdisp(); diff --git a/dlls/vbscript/vbscript.c b/dlls/vbscript/vbscript.c index ff28b3b17ef..23236f9a2e2 100644 --- a/dlls/vbscript/vbscript.c +++ b/dlls/vbscript/vbscript.c @@ -261,6 +261,54 @@ named_item_t *lookup_named_item(script_ctx_t *ctx, const WCHAR *name, unsigned f return NULL; } +typedef struct { + struct rb_entry entry; + DISPID id; + WCHAR name[1]; +} dispid_cache_entry_t; + +static int dispid_cache_cmp(const void *key, const struct rb_entry *entry) +{ + dispid_cache_entry_t *cache_entry = RB_ENTRY_VALUE(entry, dispid_cache_entry_t, entry); + return vbs_wcsicmp(key, cache_entry->name); +} + +static void free_dispid_cache_entry(struct rb_entry *entry, void *context) +{ + free(RB_ENTRY_VALUE(entry, dispid_cache_entry_t, entry)); +} + +static void clear_dispid_cache(named_item_t *item) +{ + rb_destroy(&item->dispid_cache, free_dispid_cache_entry, NULL); + rb_init(&item->dispid_cache, dispid_cache_cmp); +} + +HRESULT named_item_get_disp_id(named_item_t *item, BSTR name, vbdisp_invoke_type_t invoke_type, BOOL search_private, DISPID *id) +{ + dispid_cache_entry_t *cache_entry; + struct rb_entry *entry; + size_t name_size; + HRESULT hres; + + if((entry = rb_get(&item->dispid_cache, name))) { + *id = RB_ENTRY_VALUE(entry, dispid_cache_entry_t, entry)->id; + return S_OK; + } + + hres = disp_get_id(item->disp, name, invoke_type, search_private, id); + if(FAILED(hres)) + return hres; + + name_size = (lstrlenW(name) + 1) * sizeof(WCHAR); + if((cache_entry = malloc(offsetof(dispid_cache_entry_t, name[0]) + name_size))) { + memcpy(cache_entry->name, name, name_size); + cache_entry->id = *id; + rb_put(&item->dispid_cache, cache_entry->name, &cache_entry->entry); + } + return S_OK; +} + static void release_named_item_script_obj(named_item_t *item) { if(!item->script_obj) return; @@ -274,6 +322,7 @@ void release_named_item(named_item_t *item) { if(--item->ref) return; + clear_dispid_cache(item); free(item->name); free(item); } @@ -307,6 +356,7 @@ static void release_script(script_ctx_t *ctx) { IDispatch_Release(item->disp); item->disp = NULL; + clear_dispid_cache(item); } release_named_item_script_obj(item); if(!(item->flags & SCRIPTITEM_ISPERSISTENT)) @@ -869,6 +919,7 @@ static HRESULT WINAPI VBScript_AddNamedItem(IActiveScript *iface, LPCOLESTR pstr item->disp = disp; item->flags = dwFlags; item->script_obj = NULL; + rb_init(&item->dispid_cache, dispid_cache_cmp); item->name = wcsdup(pstrName); if(!item->name) { if(disp) diff --git a/dlls/vbscript/vbscript.h b/dlls/vbscript/vbscript.h index e3c57b0f8e5..cccfb47bfb3 100644 --- a/dlls/vbscript/vbscript.h +++ b/dlls/vbscript/vbscript.h @@ -164,9 +164,14 @@ typedef struct named_item_t { DWORD flags; LPWSTR name; + /* member names resolved against disp, valid while disp is */ + struct rb_tree dispid_cache; + struct list entry; } named_item_t; +HRESULT named_item_get_disp_id(named_item_t*,BSTR,vbdisp_invoke_type_t,BOOL,DISPID*); + HRESULT create_vbdisp(const class_desc_t*,vbdisp_t**); HRESULT disp_get_id(IDispatch*,BSTR,vbdisp_invoke_type_t,BOOL,DISPID*); const WCHAR *vbdisp_class_name(IDispatch*); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11136
The idea seems correct, but I wonder if adding yet another tree is the right direction. We already have separate trees for variables and functions in `ScriptDisp`; this change would make it three trees with exclusive keys. Maybe we should consolidate into a single tree instead, storing both the entry type (variable, function or host property) and a payload that depends on it (`dynamic_var_t *`, `function_t *` or `DISPID`). I suspect that what you observed in !10393 is a result of populating this kind of tree when linking global code. Further down the road, this type of unified representation could make global variable binding much easier. The compiler could emit local indexes that we would then bind to a corresponding tree entry in `exec_global_code`. It should be also a bit faster. Does that seem reasonable? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11136#note_143365
Thanks, that makes sense as an end state and I agree a unified, tagged representation would help global variable binding (and likely what I ran into in !10393). One thing worth flagging about this MR specifically: the new tree isn't a third tree alongside the two in `ScriptDisp`. The `var_tree` and `func_tree` live on `ScriptDisp` and hold global script-defined symbols, whereas the `dispid_cache` this MR adds lives on `named_item_t`: one per host item, keyed by host member name, holding `DISPID`s resolved through that item's `IDispatch`, and dropped together with that item's dispatch. So it has a different owner and a different lifetime than the `ScriptDisp` trees, and it wouldn't fold into them directly: consolidating `var_tree` + `func_tree` into one tagged tree (variable / function / host property -> payload) is a separate change from caching per-named-item host `DISPID`s. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11136#note_143587
But wouldn't having it in the unified tree have the same effect? On top of !11196, we could replace the lookup_global_vars and lookup_global_funcs pair in lookup_identifier with a single lookup and then make it understand the host property. If we end up finding a property via a host dispatch, we would allocate a slot in the unified tree. A subsequent lookup would then find the variable in the tree directly, so it'd never search the host dispatch again. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11136#note_143648
This merge request was closed by Francis De Brabandere. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11136
Closing in favor of !11211 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11136#note_143742
participants (3)
-
Francis De Brabandere -
Francis De Brabandere (＠francisdb)
-
Jacek Caban (＠jacek)