From: Gabriel Ivăncescu gabrielopcode@gmail.com
Objects that have had a weakref will become "zombies" with zero refcount and gc_marked (to identify them as garbage) but not part of the objects list, until we clean them up during a garbage collector run. It also doesn't hurt performance like scanning the WeakMaps on every such object's release would, and the object is otherwise completely unlinked anyway, so it doesn't keep unnecessary refs in the meantime.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/jscript/dispex.c | 127 +++++++++++++++++++++++++++++++++++++++- dlls/jscript/error.c | 1 + dlls/jscript/jscript.c | 1 + dlls/jscript/jscript.h | 6 ++ dlls/jscript/jscript.rc | 1 + dlls/jscript/resource.h | 1 + dlls/jscript/set.c | 115 +++++++++++++++++++++++++++++++++++- 7 files changed, 246 insertions(+), 6 deletions(-)
diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index f2e5109352e..8300e2ccec0 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -714,12 +714,20 @@ static void unlink_jsdisp(jsdisp_t *jsdisp) * that the only remaining refcount on each object is the number of "external refs" to it. * At the same time, mark all of the objects so that they can be potentially collected. * + * We also build a list of WeakMaps during this iteration for the next step. + * * 2. For each object with a non-zero "external refcount", clear the mark from step 1, and * recursively traverse all linked objects from it, clearing their marks as well (regardless * of their refcount), stopping a given path when the object is unmarked (and then going back * up the GC stack). This basically unmarks all of the objects with "external refcounts" * and those accessible from them, and only the leaked dangling objects will still be marked. * + * Note that we do *not* traverse WeakMaps when traversing the other objects. We need to do at + * least one additional pass over WeakMaps next, unmarking all objects that have live keys and + * are part of a live WeakMap, since we need both the key and the WeakMap to actually be able + * to access the value. But we have to repeat this iteration over WeakMaps as long as we have + * unmarked at least one object in the process, since it can make new paths alive for them. + * * 3. For each object that is marked, unlink all of the objects linked from it, because they * are dangling in a circular refcount and not accessible. This should release them. * @@ -731,15 +739,21 @@ static void unlink_jsdisp(jsdisp_t *jsdisp) * has to be a balance between reclaiming dangling objects and performance. * */ +struct gc_weakmap_list { + jsdisp_t *weakmaps[252]; + struct gc_weakmap_list *prev; +}; + struct gc_stack_chunk { jsdisp_t *objects[1020]; struct gc_stack_chunk *prev; };
struct gc_ctx { + struct gc_weakmap_list *weakmaps; struct gc_stack_chunk *chunk; struct gc_stack_chunk *next; - unsigned idx; + unsigned idx, weakmap_idx; };
static HRESULT gc_stack_push(struct gc_ctx *gc_ctx, jsdisp_t *obj) @@ -841,6 +855,45 @@ HRESULT gc_unmark(struct gc_ctx *gc_ctx, jsdisp_t *obj) return hres; }
+static void gc_free_dead_weak_refs(struct gc_ctx *gc_ctx, script_ctx_t *ctx) +{ + jsdisp_t *obj, *obj2; + + /* Only bother if we actually had any weak maps in the first place, but note that + we can't rely on the weakmaps list in the gc_ctx, even if it was still available, + because some of them might have been destroyed already during unlinking. */ + if(gc_ctx->weakmaps) { + struct list *iter = list_head(&ctx->objects); + while(iter) { + obj = LIST_ENTRY(iter, jsdisp_t, entry); + if(!obj->builtin_info->free_dead_weak_refs) { + iter = list_next(&ctx->objects, iter); + continue; + } + + /* Grab it since it could get released when dead entries get removed */ + jsdisp_addref(obj); + obj->builtin_info->free_dead_weak_refs(obj); + + /* If releasing it now causes the object to be destroyed, unlink it + first so we can safely obtain the next pointer before releasing it. */ + if(obj->ref == 1) + unlink_jsdisp(obj); + + iter = list_next(&ctx->objects, iter); + jsdisp_release(obj); + } + } + + LIST_FOR_EACH_ENTRY_SAFE(obj, obj2, &ctx->zombie_objects, jsdisp_t, entry) { + if(obj->builtin_info->destructor) + obj->builtin_info->destructor(obj); + else + free(obj); + } + list_init(&ctx->zombie_objects); +} + HRESULT gc_run(script_ctx_t *ctx) { /* Save original refcounts in a linked list of chunks */ @@ -849,9 +902,11 @@ HRESULT gc_run(script_ctx_t *ctx) struct chunk *next; LONG ref[1020]; } *head, *chunk; + struct gc_weakmap_list *weakmaps; dispex_prop_t *prop, *props_end; struct gc_ctx gc_ctx = { 0 }; unsigned chunk_idx = 0; + BOOL objects_unmarked; jsdisp_t *obj, *link; HRESULT hres = S_OK; struct list *iter; @@ -865,6 +920,9 @@ HRESULT gc_run(script_ctx_t *ctx) head->next = NULL; chunk = head;
+ /* Init it so it allocates the first chunk properly, when needed */ + gc_ctx.weakmap_idx = ARRAY_SIZE(gc_ctx.weakmaps->weakmaps); + /* 1. Save actual refcounts and decrease them speculatively as-if we unlinked the objects */ LIST_FOR_EACH_ENTRY(obj, &ctx->objects, jsdisp_t, entry) { if(chunk_idx == ARRAY_SIZE(chunk->ref)) { @@ -901,8 +959,11 @@ HRESULT gc_run(script_ctx_t *ctx)
if(obj->prototype && obj->prototype->ctx == ctx) obj->prototype->ref--; - if(obj->builtin_info->gc_traverse) - obj->builtin_info->gc_traverse(&gc_ctx, GC_TRAVERSE_SPECULATIVELY, obj); + if(obj->builtin_info->gc_traverse) { + hres = obj->builtin_info->gc_traverse(&gc_ctx, GC_TRAVERSE_SPECULATIVELY, obj); + if(FAILED(hres)) + goto restore; + } obj->gc_marked = TRUE; }
@@ -915,6 +976,31 @@ HRESULT gc_run(script_ctx_t *ctx) if(FAILED(hres)) break; } + + do { + unsigned idx = gc_ctx.weakmap_idx; + + weakmaps = gc_ctx.weakmaps; + objects_unmarked = FALSE; + + while(weakmaps) { + do { + jsdisp_t *weakmap = weakmaps->weakmaps[--idx]; + + if(weakmap->gc_marked) + continue; + hres = gc_traverse_weakmap(&gc_ctx, weakmap); + if(FAILED(hres)) + goto restore; + objects_unmarked |= (hres == S_OK); + } while(idx); + + idx = ARRAY_SIZE(weakmaps->weakmaps); + weakmaps = weakmaps->prev; + } + } while(objects_unmarked); + +restore: free(gc_ctx.next);
/* Restore */ @@ -929,6 +1015,13 @@ HRESULT gc_run(script_ctx_t *ctx) } free(chunk);
+ weakmaps = gc_ctx.weakmaps; + while(weakmaps) { + struct gc_weakmap_list *tmp = weakmaps; + weakmaps = tmp->prev; + free(tmp); + } + if(FAILED(hres)) return hres;
@@ -953,11 +1046,28 @@ HRESULT gc_run(script_ctx_t *ctx) jsdisp_release(obj); }
+ gc_free_dead_weak_refs(&gc_ctx, ctx); ctx->gc_is_unlinking = FALSE; ctx->gc_last_tick = GetTickCount(); return S_OK; }
+HRESULT gc_note_weakmap(struct gc_ctx *gc_ctx, jsdisp_t *weakmap) +{ + if(gc_ctx->weakmap_idx == ARRAY_SIZE(gc_ctx->weakmaps->weakmaps)) { + struct gc_weakmap_list *new_chunk = malloc(sizeof(*new_chunk)); + + if(!new_chunk) + return E_OUTOFMEMORY; + + new_chunk->prev = gc_ctx->weakmaps; + gc_ctx->weakmaps = new_chunk; + gc_ctx->weakmap_idx = 0; + } + gc_ctx->weakmaps->weakmaps[gc_ctx->weakmap_idx++] = weakmap; + return S_OK; +} + HRESULT gc_process_linked_obj(struct gc_ctx *gc_ctx, enum gc_traverse_op op, jsdisp_t *obj, jsdisp_t *link, void **unlink_ref) { if(op == GC_TRAVERSE_UNLINK) { @@ -2254,6 +2364,17 @@ void jsdisp_free(jsdisp_t *obj) if(obj->prototype) jsdisp_release(obj->prototype);
+ /* If we possibly have weak refs to this object, keep it around as a zombie, + so that it only gets actually freed after the weak refs are removed. + Unlink it first so it doesn't hold pointless refs to other objects. */ + if(obj->has_weak_refs) { + if(obj->builtin_info->gc_traverse) + obj->builtin_info->gc_traverse(NULL, GC_TRAVERSE_UNLINK, obj); + list_add_tail(&obj->ctx->zombie_objects, &obj->entry); + obj->gc_marked = TRUE; + return; + } + if(obj->builtin_info->destructor) obj->builtin_info->destructor(obj); else diff --git a/dlls/jscript/error.c b/dlls/jscript/error.c index c3768668178..cf0ece42f15 100644 --- a/dlls/jscript/error.c +++ b/dlls/jscript/error.c @@ -483,6 +483,7 @@ jsdisp_t *create_builtin_error(script_ctx_t *ctx) case JS_E_OBJECT_NONEXTENSIBLE: case JS_E_NONCONFIGURABLE_REDEFINED: case JS_E_NONWRITABLE_MODIFIED: + case JS_E_KEY_NOT_OBJECT: case JS_E_PROP_DESC_MISMATCH: case JS_E_INVALID_WRITABLE_PROP_DESC: constr = ctx->type_error_constr; diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 401f6ca85b0..8896fff720c 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -748,6 +748,7 @@ static HRESULT WINAPI JScript_SetScriptSite(IActiveScript *iface, ctx->acc = jsval_undefined(); list_init(&ctx->named_items); list_init(&ctx->objects); + list_init(&ctx->zombie_objects); heap_pool_init(&ctx->tmp_heap);
hres = create_jscaller(ctx); diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 71f2e2f38bd..d32528aa090 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -151,6 +151,8 @@ named_item_t *lookup_named_item(script_ctx_t*,const WCHAR*,unsigned); void release_named_item(named_item_t*); HRESULT gc_run(script_ctx_t*); HRESULT gc_unmark(struct gc_ctx*,jsdisp_t*); +HRESULT gc_note_weakmap(struct gc_ctx*,jsdisp_t*); +HRESULT gc_traverse_weakmap(struct gc_ctx*,jsdisp_t*); HRESULT gc_process_linked_obj(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsdisp_t*,void**); HRESULT gc_process_linked_val(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*,jsval_t*);
@@ -173,6 +175,7 @@ typedef struct { HRESULT (*idx_get)(jsdisp_t*,unsigned,jsval_t*); HRESULT (*idx_put)(jsdisp_t*,unsigned,jsval_t); HRESULT (*gc_traverse)(struct gc_ctx*,enum gc_traverse_op,jsdisp_t*); + void (*free_dead_weak_refs)(jsdisp_t*); } builtin_info_t;
struct jsdisp_t { @@ -180,6 +183,7 @@ struct jsdisp_t {
LONG ref;
+ BOOLEAN has_weak_refs; BOOLEAN extensible; BOOLEAN gc_marked;
@@ -372,6 +376,7 @@ struct _script_ctx_t { struct _call_frame_t *call_ctx; struct list named_items; struct list objects; + struct list zombie_objects; IActiveScriptSite *site; IInternetHostSecurityManager *secmgr; DWORD safeopt; @@ -551,6 +556,7 @@ static inline HRESULT disp_call_value(script_ctx_t *ctx, IDispatch *disp, jsval_ #define JS_E_NONCONFIGURABLE_REDEFINED MAKE_JSERROR(IDS_NONCONFIGURABLE_REDEFINED) #define JS_E_NONWRITABLE_MODIFIED MAKE_JSERROR(IDS_NONWRITABLE_MODIFIED) #define JS_E_WRONG_THIS MAKE_JSERROR(IDS_WRONG_THIS) +#define JS_E_KEY_NOT_OBJECT MAKE_JSERROR(IDS_KEY_NOT_OBJECT) #define JS_E_PROP_DESC_MISMATCH MAKE_JSERROR(IDS_PROP_DESC_MISMATCH) #define JS_E_INVALID_WRITABLE_PROP_DESC MAKE_JSERROR(IDS_INVALID_WRITABLE_PROP_DESC)
diff --git a/dlls/jscript/jscript.rc b/dlls/jscript/jscript.rc index fbf965fb7e8..e5289a550a3 100644 --- a/dlls/jscript/jscript.rc +++ b/dlls/jscript/jscript.rc @@ -77,6 +77,7 @@ STRINGTABLE IDS_NONCONFIGURABLE_REDEFINED "Cannot redefine non-configurable property '|'" IDS_NONWRITABLE_MODIFIED "Cannot modify non-writable property '|'" IDS_WRONG_THIS "'this' is not a | object" + IDS_KEY_NOT_OBJECT "'key' is not an object" IDS_PROP_DESC_MISMATCH "Property cannot have both accessors and a value"
IDS_COMPILATION_ERROR "Microsoft JScript compilation error" diff --git a/dlls/jscript/resource.h b/dlls/jscript/resource.h index 0ac457d740d..ad771b67475 100644 --- a/dlls/jscript/resource.h +++ b/dlls/jscript/resource.h @@ -75,6 +75,7 @@ #define IDS_NONCONFIGURABLE_REDEFINED 0x13D6 #define IDS_NONWRITABLE_MODIFIED 0x13D7 #define IDS_WRONG_THIS 0x13FC +#define IDS_KEY_NOT_OBJECT 0x13FD /* FIXME: This is not compatible with native, but we would * conflict with IDS_UNSUPPORTED_ACTION otherwise */ #define IDS_PROP_DESC_MISMATCH 0x1F00 diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index f2de6924468..cdd7b50c7a9 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -631,8 +631,31 @@ static int weakmap_compare(const void *k, const struct rb_entry *e) return (a > b) - (a < b); }
+static HRESULT get_weakmap_this(script_ctx_t *ctx, jsval_t vthis, WeakMapInstance **ret) +{ + jsdisp_t *jsdisp; + + if(!is_object_instance(vthis)) + return JS_E_OBJECT_EXPECTED; + if(!(jsdisp = to_jsdisp(get_object(vthis))) || !is_class(jsdisp, JSCLASS_WEAKMAP)) { + WARN("not a WeakMap object passed as 'this'\n"); + return throw_error(ctx, JS_E_WRONG_THIS, L"WeakMap"); + } + + *ret = CONTAINING_RECORD(jsdisp, WeakMapInstance, dispex); + return S_OK; +} + +static struct weakmap_entry *get_weakmap_entry(WeakMapInstance *weakmap, jsdisp_t *key) +{ + struct rb_entry *entry; + if(!(entry = rb_get(&weakmap->map, key))) return NULL; + return CONTAINING_RECORD(entry, struct weakmap_entry, entry); +} + static void release_weakmap_entry(struct weakmap_entry *entry) { + /* We can't clear the has_weak_refs flag on the key, because there might be other weak refs */ jsval_release(entry->value); free(entry); } @@ -661,8 +684,50 @@ static HRESULT WeakMap_get(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigne static HRESULT WeakMap_set(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { - FIXME("\n"); - return E_NOTIMPL; + jsdisp_t *key = (argc >= 1 && is_object_instance(argv[0])) ? to_jsdisp(get_object(argv[0])) : NULL; + jsval_t value = argc >= 2 ? argv[1] : jsval_undefined(); + struct weakmap_entry *entry; + WeakMapInstance *weakmap; + HRESULT hres; + + hres = get_weakmap_this(ctx, vthis, &weakmap); + if(FAILED(hres)) + return hres; + + TRACE("%p (%p %s)\n", weakmap, key, debugstr_jsval(value)); + + if(!key) + return JS_E_KEY_NOT_OBJECT; + + if(key->ctx != ctx) { + FIXME("different ctx not supported\n"); + return JS_E_KEY_NOT_OBJECT; + } + + if((entry = get_weakmap_entry(weakmap, key))) { + jsval_t val; + hres = jsval_copy(value, &val); + if(FAILED(hres)) + return hres; + + jsval_release(entry->value); + entry->value = val; + }else { + if(!(entry = malloc(sizeof(*entry)))) + return E_OUTOFMEMORY; + + entry->key = key; + hres = jsval_copy(value, &entry->value); + if(FAILED(hres)) { + free(entry); + return hres; + } + key->has_weak_refs = TRUE; + rb_put(&weakmap->map, key, &entry->entry); + } + + if(r) *r = jsval_undefined(); + return S_OK; }
static HRESULT WeakMap_has(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv, @@ -703,14 +768,57 @@ static HRESULT WeakMap_gc_traverse(struct gc_ctx *gc_ctx, enum gc_traverse_op op return S_OK; }
+ /* Don't traverse the values here, that's done in a separate iteration in the GC. */ + if(op == GC_TRAVERSE) + return S_OK; + + /* For speculative traversal, the behavior is identical so it is OK to process them normally. + However, we do have to add us to the list of WeakMaps for the GC to traverse later. */ + assert(op == GC_TRAVERSE_SPECULATIVELY); RB_FOR_EACH_ENTRY(entry, &weakmap->map, struct weakmap_entry, entry) { - hres = gc_process_linked_val(gc_ctx, op, dispex, &entry->value); + gc_process_linked_val(gc_ctx, op, dispex, &entry->value); + hres = gc_note_weakmap(gc_ctx, &weakmap->dispex); if(FAILED(hres)) return hres; } return S_OK; }
+HRESULT gc_traverse_weakmap(struct gc_ctx *gc_ctx, jsdisp_t *dispex) +{ + WeakMapInstance *weakmap = (WeakMapInstance*)dispex; + script_ctx_t *ctx = weakmap->dispex.ctx; + struct weakmap_entry *entry; + HRESULT hres = S_FALSE; + jsdisp_t *jsdisp; + + /* Here we need to traverse the WeakMap and unmark all objects found from its live keys */ + RB_FOR_EACH_ENTRY(entry, &weakmap->map, struct weakmap_entry, entry) { + if(!is_object_instance(entry->value) || !(jsdisp = to_jsdisp(get_object(entry->value)))) + continue; + + if(jsdisp->gc_marked && !entry->key->gc_marked && jsdisp->ctx == ctx) { + hres = gc_unmark(gc_ctx, jsdisp); + if(FAILED(hres)) + return hres; + } + } + return hres; +} + +static void WeakMap_free_dead_weak_refs(jsdisp_t *dispex) +{ + WeakMapInstance *weakmap = (WeakMapInstance*)dispex; + struct weakmap_entry *entry, *entry2; + + RB_FOR_EACH_ENTRY_DESTRUCTOR(entry, entry2, &weakmap->map, struct weakmap_entry, entry) { + if(entry->key->gc_marked) { + rb_remove(&weakmap->map, &entry->entry); + release_weakmap_entry(entry); + } + } +} + static const builtin_prop_t WeakMap_prototype_props[] = { {L"clear", WeakMap_clear, PROPF_METHOD}, {L"delete", WeakMap_delete, PROPF_METHOD|1}, @@ -739,6 +847,7 @@ static const builtin_info_t WeakMap_info = { NULL, NULL, WeakMap_gc_traverse, + WeakMap_free_dead_weak_refs };
static HRESULT WeakMap_constructor(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned argc, jsval_t *argv,