From b1618db0726d9f524e2b8842f6a26c731dffd71e Mon Sep 17 00:00:00 2001 From: Dmitry Panov Date: Tue, 5 Jul 2022 10:58:43 +0100 Subject: [PATCH] Implemented 'copy-on-change' mechanism for inner compound values. Fixes #403. --- array.go | 16 +-- array_sparse.go | 12 +- builtin_array.go | 21 ++- builtin_set.go | 8 +- destruct.go | 6 +- object.go | 16 +-- object_dynamic.go | 16 +-- object_goarray_reflect.go | 120 ++++++++++++++--- object_goarray_reflect_test.go | 228 ++++++++++++++++++++++++++++++++- object_gomap_reflect.go | 11 +- object_gomap_reflect_test.go | 20 +++ object_goreflect.go | 113 ++++++++++++++-- object_goreflect_test.go | 60 +++++++++ object_goslice.go | 58 ++++++--- object_goslice_reflect.go | 17 ++- object_goslice_reflect_test.go | 45 +++++++ object_goslice_test.go | 16 +++ object_lazy.go | 6 +- runtime.go | 128 ++++++++++-------- 19 files changed, 749 insertions(+), 168 deletions(-) diff --git a/array.go b/array.go index f8028d27..34d122f9 100644 --- a/array.go +++ b/array.go @@ -178,11 +178,11 @@ func (a *arrayObject) getOwnPropIdx(idx valueInt) Value { return a.baseObject.getOwnPropStr(idx.string()) } -func (a *arrayObject) sortLen() int64 { - return int64(len(a.values)) +func (a *arrayObject) sortLen() int { + return len(a.values) } -func (a *arrayObject) sortGet(i int64) Value { +func (a *arrayObject) sortGet(i int) Value { v := a.values[i] if p, ok := v.(*valueProperty); ok { v = p.get(a.val) @@ -190,7 +190,7 @@ func (a *arrayObject) sortGet(i int64) Value { return v } -func (a *arrayObject) swap(i, j int64) { +func (a *arrayObject) swap(i int, j int) { a.values[i], a.values[j] = a.values[j], a.values[i] } @@ -511,12 +511,12 @@ func (a *arrayObject) exportToArrayOrSlice(dst reflect.Value, typ reflect.Type, r := a.val.runtime if iter := a.getSym(SymIterator, nil); iter == r.global.arrayValues || iter == nil { l := toIntStrict(int64(a.length)) - if dst.Len() != l { - if typ.Kind() == reflect.Array { + if typ.Kind() == reflect.Array { + if dst.Len() != l { return fmt.Errorf("cannot convert an Array into an array, lengths mismatch (have %d, need %d)", l, dst.Len()) - } else { - dst.Set(reflect.MakeSlice(typ, l, l)) } + } else { + dst.Set(reflect.MakeSlice(typ, l, l)) } ctx.putTyped(a.val, typ, dst.Interface()) for i := 0; i < l; i++ { diff --git a/array_sparse.go b/array_sparse.go index 9a352aff..201bc6fa 100644 --- a/array_sparse.go +++ b/array_sparse.go @@ -409,9 +409,9 @@ func (a *sparseArrayObject) deleteIdx(idx valueInt, throw bool) bool { return a.baseObject.deleteStr(idx.string(), throw) } -func (a *sparseArrayObject) sortLen() int64 { +func (a *sparseArrayObject) sortLen() int { if len(a.items) > 0 { - return int64(a.items[len(a.items)-1].idx) + 1 + return toIntStrict(int64(a.items[len(a.items)-1].idx) + 1) } return 0 @@ -460,12 +460,12 @@ func (a *sparseArrayObject) exportToArrayOrSlice(dst reflect.Value, typ reflect. r := a.val.runtime if iter := a.getSym(SymIterator, nil); iter == r.global.arrayValues || iter == nil { l := toIntStrict(int64(a.length)) - if dst.Len() != l { - if typ.Kind() == reflect.Array { + if typ.Kind() == reflect.Array { + if dst.Len() != l { return fmt.Errorf("cannot convert an Array into an array, lengths mismatch (have %d, need %d)", l, dst.Len()) - } else { - dst.Set(reflect.MakeSlice(typ, l, l)) } + } else { + dst.Set(reflect.MakeSlice(typ, l, l)) } ctx.putTyped(a.val, typ, dst.Interface()) for _, item := range a.items { diff --git a/builtin_array.go b/builtin_array.go index 08bd4932..89847067 100644 --- a/builtin_array.go +++ b/builtin_array.go @@ -352,9 +352,16 @@ func (r *Runtime) arrayproto_sort(call FunctionCall) Value { } } + var s sortable if r.checkStdArrayObj(o) != nil { + s = o.self + } else if _, ok := o.self.(reflectValueWrapper); ok { + s = o.self + } + + if s != nil { ctx := arraySortCtx{ - obj: o.self, + obj: s, compare: compareFn, } @@ -1443,9 +1450,9 @@ func (r *Runtime) initArray() { } type sortable interface { - sortLen() int64 - sortGet(int64) Value - swap(int64, int64) + sortLen() int + sortGet(int) Value + swap(int, int) } type arraySortCtx struct { @@ -1500,13 +1507,13 @@ func (a *arraySortCtx) sortCompare(x, y Value) int { // sort.Interface func (a *arraySortCtx) Len() int { - return int(a.obj.sortLen()) + return a.obj.sortLen() } func (a *arraySortCtx) Less(j, k int) bool { - return a.sortCompare(a.obj.sortGet(int64(j)), a.obj.sortGet(int64(k))) < 0 + return a.sortCompare(a.obj.sortGet(j), a.obj.sortGet(k)) < 0 } func (a *arraySortCtx) Swap(j, k int) { - a.obj.swap(int64(j), int64(k)) + a.obj.swap(j, k) } diff --git a/builtin_set.go b/builtin_set.go index 72e9015f..e554b75c 100644 --- a/builtin_set.go +++ b/builtin_set.go @@ -65,12 +65,12 @@ func (so *setObject) export(ctx *objectExportCtx) interface{} { func (so *setObject) exportToArrayOrSlice(dst reflect.Value, typ reflect.Type, ctx *objectExportCtx) error { l := so.m.size - if dst.Len() != l { - if typ.Kind() == reflect.Array { + if typ.Kind() == reflect.Array { + if dst.Len() != l { return fmt.Errorf("cannot convert a Set into an array, lengths mismatch: have %d, need %d)", l, dst.Len()) - } else { - dst.Set(reflect.MakeSlice(typ, l, l)) } + } else { + dst.Set(reflect.MakeSlice(typ, l, l)) } ctx.putTyped(so.val, typ, dst.Interface()) iter := so.m.newIter() diff --git a/destruct.go b/destruct.go index a9ef1186..d24685fe 100644 --- a/destruct.go +++ b/destruct.go @@ -36,15 +36,15 @@ func (d *destructKeyedSource) recordKey(key Value) { d.usedKeys[key] = struct{}{} } -func (d *destructKeyedSource) sortLen() int64 { +func (d *destructKeyedSource) sortLen() int { return d.w().sortLen() } -func (d *destructKeyedSource) sortGet(i int64) Value { +func (d *destructKeyedSource) sortGet(i int) Value { return d.w().sortGet(i) } -func (d *destructKeyedSource) swap(i int64, i2 int64) { +func (d *destructKeyedSource) swap(i int, i2 int) { d.w().swap(i, i2) } diff --git a/object.go b/object.go index d3fc45f6..63a175d9 100644 --- a/object.go +++ b/object.go @@ -923,15 +923,15 @@ func (o *baseObject) preventExtensions(bool) bool { return true } -func (o *baseObject) sortLen() int64 { - return toLength(o.val.self.getStr("length", nil)) +func (o *baseObject) sortLen() int { + return toIntStrict(toLength(o.val.self.getStr("length", nil))) } -func (o *baseObject) sortGet(i int64) Value { +func (o *baseObject) sortGet(i int) Value { return o.val.self.getIdx(valueInt(i), nil) } -func (o *baseObject) swap(i, j int64) { +func (o *baseObject) swap(i int, j int) { ii := valueInt(i) jj := valueInt(j) @@ -1026,12 +1026,12 @@ func genericExportToArrayOrSlice(o *Object, dst reflect.Value, typ reflect.Type, if ex != nil { return ex } - if dst.Len() != len(values) { - if typ.Kind() == reflect.Array { + if typ.Kind() == reflect.Array { + if dst.Len() != len(values) { return fmt.Errorf("cannot convert an iterable into an array, lengths mismatch (have %d, need %d)", len(values), dst.Len()) - } else { - dst.Set(reflect.MakeSlice(typ, len(values), len(values))) } + } else { + dst.Set(reflect.MakeSlice(typ, len(values), len(values))) } ctx.putTyped(o, typ, dst.Interface()) for i, val := range values { diff --git a/object_dynamic.go b/object_dynamic.go index e1ef97d5..475f80f8 100644 --- a/object_dynamic.go +++ b/object_dynamic.go @@ -132,15 +132,15 @@ func (r *Runtime) NewDynamicArray(a DynamicArray) *Object { return v } -func (*dynamicObject) sortLen() int64 { +func (*dynamicObject) sortLen() int { return 0 } -func (*dynamicObject) sortGet(i int64) Value { +func (*dynamicObject) sortGet(i int) Value { return nil } -func (*dynamicObject) swap(i int64, i2 int64) { +func (*dynamicObject) swap(i int, i2 int) { } func (*dynamicObject) className() string { @@ -526,15 +526,15 @@ func (*baseDynamicObject) _putProp(name unistring.String, value Value, writable, func (*baseDynamicObject) _putSym(s *Symbol, prop Value) { } -func (a *dynamicArray) sortLen() int64 { - return int64(a.a.Len()) +func (a *dynamicArray) sortLen() int { + return a.a.Len() } -func (a *dynamicArray) sortGet(i int64) Value { - return a.a.Get(int(i)) +func (a *dynamicArray) sortGet(i int) Value { + return a.a.Get(i) } -func (a *dynamicArray) swap(i int64, j int64) { +func (a *dynamicArray) swap(i int, j int) { x := a.sortGet(i) y := a.sortGet(j) a.a.Set(int(i), y) diff --git a/object_goarray_reflect.go b/object_goarray_reflect.go index 92f9e958..3eb6bb8c 100644 --- a/object_goarray_reflect.go +++ b/object_goarray_reflect.go @@ -11,9 +11,51 @@ type objectGoArrayReflect struct { objectGoReflect lengthProp valueProperty + valueCache valueArrayCache + putIdx func(idx int, v Value, throw bool) bool } +type valueArrayCache []reflectValueWrapper + +func (c *valueArrayCache) get(idx int) reflectValueWrapper { + if idx < len(*c) { + return (*c)[idx] + } + return nil +} + +func (c *valueArrayCache) grow(newlen int) { + oldcap := cap(*c) + if oldcap < newlen { + a := make([]reflectValueWrapper, newlen, growCap(newlen, len(*c), oldcap)) + copy(a, *c) + *c = a + } else { + *c = (*c)[:newlen] + } +} + +func (c *valueArrayCache) put(idx int, w reflectValueWrapper) { + if len(*c) <= idx { + c.grow(idx + 1) + } + (*c)[idx] = w +} + +func (c *valueArrayCache) shrink(newlen int) { + if len(*c) > newlen { + tail := (*c)[newlen:] + for i, item := range tail { + if item != nil { + copyReflectValueWrapper(item) + tail[i] = nil + } + } + *c = (*c)[:newlen] + } +} + func (o *objectGoArrayReflect) _init() { o.objectGoReflect.init() o.class = classArray @@ -46,11 +88,18 @@ func (o *objectGoArrayReflect) _hasStr(name unistring.String) bool { } func (o *objectGoArrayReflect) _getIdx(idx int) Value { + if v := o.valueCache.get(idx); v != nil { + return v.esValue() + } + v := o.value.Index(idx) - if (v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface) && v.IsNil() { - return _null + + res, w := o.elemToValue(v) + if w != nil { + o.valueCache.put(idx, w) } - return o.val.runtime.toValue(v.Interface(), v) + + return res } func (o *objectGoArrayReflect) getIdx(idx valueInt, receiver Value) Value { @@ -101,11 +150,23 @@ func (o *objectGoArrayReflect) getOwnPropIdx(idx valueInt) Value { } func (o *objectGoArrayReflect) _putIdx(idx int, v Value, throw bool) bool { - err := o.val.runtime.toReflectValue(v, o.value.Index(idx), &objectExportCtx{}) + cached := o.valueCache.get(idx) + if cached != nil { + copyReflectValueWrapper(cached) + } + + rv := o.value.Index(idx) + err := o.val.runtime.toReflectValue(v, rv, &objectExportCtx{}) if err != nil { + if cached != nil { + cached.setReflectValue(rv) + } o.val.runtime.typeErrorResult(throw, "Go type conversion error: %v", err) return false } + if cached != nil { + o.valueCache[idx] = nil + } return true } @@ -211,6 +272,11 @@ func (o *objectGoArrayReflect) toPrimitive() Value { func (o *objectGoArrayReflect) _deleteIdx(idx int) { if idx < o.value.Len() { + if cv := o.valueCache.get(idx); cv != nil { + copyReflectValueWrapper(cv) + o.valueCache[idx] = nil + } + o.value.Index(idx).Set(reflect.Zero(o.value.Type().Elem())) } } @@ -262,27 +328,39 @@ func (o *objectGoArrayReflect) iterateStringKeys() iterNextFunc { }).next } -func (o *objectGoArrayReflect) equal(other objectImpl) bool { - if other, ok := other.(*objectGoArrayReflect); ok { - return o.value.Interface() == other.value.Interface() - } - return false +func (o *objectGoArrayReflect) sortLen() int { + return o.value.Len() } -func (o *objectGoArrayReflect) sortLen() int64 { - return int64(o.value.Len()) -} - -func (o *objectGoArrayReflect) sortGet(i int64) Value { +func (o *objectGoArrayReflect) sortGet(i int) Value { return o.getIdx(valueInt(i), nil) } -func (o *objectGoArrayReflect) swap(i, j int64) { - ii := toIntStrict(i) - jj := toIntStrict(j) - x := o._getIdx(ii) - y := o._getIdx(jj) +func (o *objectGoArrayReflect) swap(i int, j int) { + vi := o.value.Index(i) + vj := o.value.Index(j) + tmp := reflect.New(o.value.Type().Elem()).Elem() + tmp.Set(vi) + vi.Set(vj) + vj.Set(tmp) - o._putIdx(ii, y, false) - o._putIdx(jj, x, false) + cachedI := o.valueCache.get(i) + cachedJ := o.valueCache.get(j) + if cachedI != nil { + cachedI.setReflectValue(vj) + o.valueCache.put(j, cachedI) + } else { + if j < len(o.valueCache) { + o.valueCache[j] = nil + } + } + + if cachedJ != nil { + cachedJ.setReflectValue(vi) + o.valueCache.put(i, cachedJ) + } else { + if i < len(o.valueCache) { + o.valueCache[i] = nil + } + } } diff --git a/object_goarray_reflect_test.go b/object_goarray_reflect_test.go index ff795fef..451f36f2 100644 --- a/object_goarray_reflect_test.go +++ b/object_goarray_reflect_test.go @@ -1,6 +1,8 @@ package goja -import "testing" +import ( + "testing" +) func TestGoReflectArray(t *testing.T) { vm := New() @@ -47,3 +49,227 @@ func TestGoReflectArraySort(t *testing.T) { t.Fatalf("Wrong type: %T", res) } } + +func TestGoReflectArrayCopyOnChange(t *testing.T) { + vm := New() + + v, err := vm.RunString(` + a => { + let tmp = a[0]; + if (tmp !== a[0]) { + throw new Error("tmp !== a[0]"); + } + + a[0] = a[1]; + if (tmp === a[0]) { + throw new Error("tmp === a[0]"); + } + if (tmp.Test !== "1") { + throw new Error("tmp.Test: " + tmp.Test + " (" + typeof tmp.Test + ")"); + } + if (a[0].Test !== "2") { + throw new Error("a[0].Test: " + a[0].Test); + } + + a[0].Test = "3"; + if (a[0].Test !== "3") { + throw new Error("a[0].Test (1): " + a[0].Test); + } + + tmp = a[0]; + tmp.Test = "4"; + if (a[0].Test !== "4") { + throw new Error("a[0].Test (2): " + a[0].Test); + } + + delete a[0]; + if (a[0] && a[0].Test !== "") { + throw new Error("a[0].Test (3): " + a[0].Test); + } + if (tmp.Test !== "4") { + throw new Error("tmp.Test (1): " + tmp.Test); + } + + a[1] = tmp; + if (a[1].Test !== "4") { + throw new Error("a[1].Test: " + a[1].Test); + } + + // grow + tmp = a[1]; + a.push(null); + if (a.length !== 3) { + throw new Error("a.length after push: " + a.length); + } + + tmp.Test = "5"; + if (a[1].Test !== "5") { + throw new Error("a[1].Test (1): " + a[1].Test); + } + + // shrink + a.length = 1; + if (a.length !== 1) { + throw new Error("a.length after shrink: " + a.length); + } + + if (tmp.Test !== "5") { + throw new Error("tmp.Test (shrink): " + tmp.Test); + } + } + `) + if err != nil { + t.Fatal(err) + } + + fn, ok := AssertFunction(v) + if !ok { + t.Fatal("Not a function") + } + + t.Run("[]struct", func(t *testing.T) { + a := []struct { + Test string + }{{"1"}, {"2"}} + _, err := fn(nil, vm.ToValue(a)) + if err != nil { + t.Fatal(err) + } + if a[0].Test != "" { + t.Fatalf("a[0]: %#v", a[0]) + } + + if a[1].Test != "4" { + t.Fatalf("a0[1]: %#v", a[1]) + } + }) + + // The copy-on-change mechanism doesn't apply to the types below because the contained values are references. + // These tests are here for completeness and to prove that the behaviour is consistent. + + t.Run("[]I", func(t *testing.T) { + type I interface { + Get() string + } + + a := []I{&testGoReflectMethod_O{Test: "1"}, &testGoReflectMethod_O{Test: "2"}} + + _, err = fn(nil, vm.ToValue(a)) + if err != nil { + t.Fatal(err) + } + }) + + t.Run("[]interface{}", func(t *testing.T) { + a := []interface{}{&testGoReflectMethod_O{Test: "1"}, &testGoReflectMethod_O{Test: "2"}} + + _, err = fn(nil, vm.ToValue(a)) + if err != nil { + t.Fatal(err) + } + }) +} + +func TestCopyOnChangeReflectSlice(t *testing.T) { + vm := New() + v, err := vm.RunString(` + s => { + s.A.push(1); + if (s.A.length !== 1) { + throw new Error("s.A.length: " + s.A.length); + } + if (s.A[0] !== 1) { + throw new Error("s.A[0]: " + s.A[0]); + } + let tmp = s.A; + if (tmp !== s.A) { + throw new Error("tmp !== s.A"); + } + s.A = [2]; + if (tmp === s.A) { + throw new Error("tmp === s.A"); + } + if (tmp[0] !== 1) { + throw new Error("tmp[0]: " + tmp[0]); + } + if (s.A[0] !== 2) { + throw new Error("s.A[0] (1): " + s.A[0]); + } + } + `) + if err != nil { + t.Fatal(err) + } + fn, ok := AssertFunction(v) + if !ok { + t.Fatal("Not a function") + } + + t.Run("[]int", func(t *testing.T) { + type S struct { + A []int + } + var s S + _, err := fn(nil, vm.ToValue(&s)) + if err != nil { + t.Fatal(err) + } + if len(s.A) != 1 { + t.Fatal(s) + } + if s.A[0] != 2 { + t.Fatal(s.A) + } + }) + + t.Run("[]interface{}", func(t *testing.T) { + type S struct { + A []interface{} + } + var s S + _, err := fn(nil, vm.ToValue(&s)) + if err != nil { + t.Fatal(err) + } + if len(s.A) != 1 { + t.Fatal(s) + } + if s.A[0] != int64(2) { + t.Fatal(s.A) + } + }) +} + +func TestCopyOnChangeSort(t *testing.T) { + a := []struct { + Test string + }{{"2"}, {"1"}} + + vm := New() + vm.Set("a", &a) + + _, err := vm.RunString(` + let a0 = a[0]; + let a1 = a[1]; + a.sort((a, b) => a.Test.localeCompare(b.Test)); + if (a[0].Test !== "1") { + throw new Error("a[0]: " + a[0]); + } + if (a[1].Test !== "2") { + throw new Error("a[1]: " + a[1]); + } + if (a0 !== a[1]) { + throw new Error("a0 !== a[1]"); + } + if (a1 !== a[0]) { + throw new Error("a1 !== a[0]"); + } + `) + if err != nil { + t.Fatal(err) + } + + if a[0].Test != "1" || a[1].Test != "2" { + t.Fatal(a) + } +} diff --git a/object_gomap_reflect.go b/object_gomap_reflect.go index 324e68fe..9c19156d 100644 --- a/object_gomap_reflect.go +++ b/object_gomap_reflect.go @@ -41,7 +41,7 @@ func (o *objectGoMapReflect) _get(n Value) Value { return nil } if v := o.value.MapIndex(key); v.IsValid() { - return o.val.runtime.toValue(v.Interface(), v) + return o.val.runtime.ToValue(v.Interface()) } return nil @@ -53,7 +53,7 @@ func (o *objectGoMapReflect) _getStr(name string) Value { return nil } if v := o.value.MapIndex(key); v.IsValid() { - return o.val.runtime.toValue(v.Interface(), v) + return o.val.runtime.ToValue(v.Interface()) } return nil @@ -264,10 +264,3 @@ func (o *objectGoMapReflect) stringKeys(_ bool, accum []Value) []Value { return accum } - -func (o *objectGoMapReflect) equal(other objectImpl) bool { - if other, ok := other.(*objectGoMapReflect); ok { - return o.value.Interface() == other.value.Interface() - } - return false -} diff --git a/object_gomap_reflect_test.go b/object_gomap_reflect_test.go index 266a268a..a8364ca5 100644 --- a/object_gomap_reflect_test.go +++ b/object_gomap_reflect_test.go @@ -272,3 +272,23 @@ func TestGoMapReflectUnicode(t *testing.T) { t.Fatalf("Unexpected value: %v", res) } } + +func TestGoMapReflectStruct(t *testing.T) { + type S struct { + Test int + } + + m := map[string]S{ + "1": {Test: 1}, + } + + vm := New() + vm.Set("m", m) + res, err := vm.RunString("m[1].Test = 2; m[1].Test") + if err != nil { + t.Fatal(err) + } + if res.Export() != int64(1) { + t.Fatal(res) + } +} diff --git a/object_goreflect.go b/object_goreflect.go index 0fde9071..55440fc6 100644 --- a/object_goreflect.go +++ b/object_goreflect.go @@ -76,12 +76,35 @@ type reflectTypeInfo struct { FieldNames, MethodNames []string } +type reflectValueWrapper interface { + esValue() Value + reflectValue() reflect.Value + setReflectValue(reflect.Value) +} + +func isContainer(k reflect.Kind) bool { + switch k { + case reflect.Struct, reflect.Slice, reflect.Array: + return true + } + return false +} + +func copyReflectValueWrapper(w reflectValueWrapper) { + v := w.reflectValue() + c := reflect.New(v.Type()).Elem() + c.Set(v) + w.setReflectValue(c) +} + type objectGoReflect struct { baseObject origValue, value reflect.Value valueTypeInfo, origValueTypeInfo *reflectTypeInfo + valueCache map[string]reflectValueWrapper + toJson func() interface{} } @@ -104,7 +127,7 @@ func (o *objectGoReflect) init() { o.class = classObject o.prototype = o.val.runtime.global.ObjectPrototype if !o.value.CanAddr() { - value := reflect.Indirect(reflect.New(o.value.Type())) + value := reflect.New(o.value.Type()).Elem() value.Set(o.value) o.origValue = value o.value = value @@ -140,8 +163,7 @@ func (o *objectGoReflect) getStr(name unistring.String, receiver Value) Value { func (o *objectGoReflect) _getField(jsName string) reflect.Value { if info, exists := o.valueTypeInfo.Fields[jsName]; exists { - v := o.value.FieldByIndex(info.Index) - return v + return o.value.FieldByIndex(info.Index) } return reflect.Value{} @@ -155,15 +177,58 @@ func (o *objectGoReflect) _getMethod(jsName string) reflect.Value { return reflect.Value{} } +func (o *objectGoReflect) elemToValue(ev reflect.Value) (Value, reflectValueWrapper) { + if isContainer(ev.Kind()) { + if ev.Type() == reflectTypeArray { + a := o.val.runtime.newObjectGoSlice(ev.Addr().Interface().(*[]interface{})) + return a.val, a + } + ret := o.val.runtime.reflectValueToValue(ev) + if obj, ok := ret.(*Object); ok { + if w, ok := obj.self.(reflectValueWrapper); ok { + return ret, w + } + } + panic("reflectValueToValue() returned a value which is not a reflectValueWrapper") + } + + for ev.Kind() == reflect.Interface { + ev = ev.Elem() + } + + if ev.Kind() == reflect.Invalid { + return _null, nil + } + + return o.val.runtime.ToValue(ev.Interface()), nil +} + +func (o *objectGoReflect) _getFieldValue(name string) Value { + if v := o.valueCache[name]; v != nil { + return v.esValue() + } + if v := o._getField(name); v.IsValid() { + res, w := o.elemToValue(v) + if w != nil { + if o.valueCache == nil { + o.valueCache = make(map[string]reflectValueWrapper) + } + o.valueCache[name] = w + } + return res + } + return nil +} + func (o *objectGoReflect) _get(name string) Value { if o.value.Kind() == reflect.Struct { - if v := o._getField(name); v.IsValid() { - return o.val.runtime.toValue(v.Interface(), v) + if ret := o._getFieldValue(name); ret != nil { + return ret } } if v := o._getMethod(name); v.IsValid() { - return o.val.runtime.toValue(v.Interface(), v) + return o.val.runtime.reflectValueToValue(v) } return nil @@ -172,10 +237,10 @@ func (o *objectGoReflect) _get(name string) Value { func (o *objectGoReflect) getOwnPropStr(name unistring.String) Value { n := name.String() if o.value.Kind() == reflect.Struct { - if v := o._getField(n); v.IsValid() { + if v := o._getFieldValue(n); v != nil { return &valueProperty{ - value: o.val.runtime.toValue(v.Interface(), v), - writable: v.CanSet(), + value: v, + writable: true, enumerable: true, } } @@ -183,7 +248,7 @@ func (o *objectGoReflect) getOwnPropStr(name unistring.String) Value { if v := o._getMethod(n); v.IsValid() { return &valueProperty{ - value: o.val.runtime.toValue(v.Interface(), v), + value: o.val.runtime.reflectValueToValue(v), enumerable: true, } } @@ -215,15 +280,22 @@ func (o *objectGoReflect) setForeignIdx(idx valueInt, val, receiver Value, throw func (o *objectGoReflect) _put(name string, val Value, throw bool) (has, ok bool) { if o.value.Kind() == reflect.Struct { if v := o._getField(name); v.IsValid() { - if !v.CanSet() { - o.val.runtime.typeErrorResult(throw, "Cannot assign to a non-addressable or read-only property %s of a host object", name) - return true, false + cached := o.valueCache[name] + if cached != nil { + copyReflectValueWrapper(cached) } + err := o.val.runtime.toReflectValue(val, v, &objectExportCtx{}) if err != nil { + if cached != nil { + cached.setReflectValue(v) + } o.val.runtime.typeErrorResult(throw, "Go struct conversion error: %v", err) return true, false } + if cached != nil { + delete(o.valueCache, name) + } return true, true } } @@ -413,11 +485,24 @@ func (o *objectGoReflect) exportType() reflect.Type { func (o *objectGoReflect) equal(other objectImpl) bool { if other, ok := other.(*objectGoReflect); ok { - return o.value.Interface() == other.value.Interface() + return o.value == other.value } return false } +func (o *objectGoReflect) reflectValue() reflect.Value { + return o.value +} + +func (o *objectGoReflect) setReflectValue(v reflect.Value) { + o.value = v + o.origValue = v +} + +func (o *objectGoReflect) esValue() Value { + return o.val +} + func (r *Runtime) buildFieldInfo(t reflect.Type, index []int, info *reflectTypeInfo) { n := t.NumField() for i := 0; i < n; i++ { diff --git a/object_goreflect_test.go b/object_goreflect_test.go index 24050922..0968704b 100644 --- a/object_goreflect_test.go +++ b/object_goreflect_test.go @@ -61,6 +61,22 @@ func TestGoReflectSet(t *testing.T) { if o.Y != "2P" { t.Fatalf("Unexpected Y: %s", o.Y) } + + r.Set("o", o) + _, err = r.RunString(SCRIPT) + if err != nil { + t.Fatal(err) + } + + if res, ok := r.Get("o").Export().(O); ok { + if res.X != 6 { + t.Fatalf("Unexpected res.X: %d", res.X) + } + + if res.Y != "2PP" { + t.Fatalf("Unexpected res.Y: %s", res.Y) + } + } } func TestGoReflectEnumerate(t *testing.T) { @@ -1184,3 +1200,47 @@ func TestGoReflectPreserveType(t *testing.T) { t.Fatal(e) } } + +func TestGoReflectCopyOnWrite(t *testing.T) { + type Inner struct { + Field int + } + type S struct { + I Inner + } + var s S + s.I.Field = 1 + + vm := New() + vm.Set("s", &s) + _, err := vm.RunString(` + if (s.I.Field !== 1) { + throw new Error("s.I.Field: " + s.I.Field); + } + + let tmp = s.I; // tmp becomes a reference to s.I + if (tmp.Field !== 1) { + throw new Error("tmp.Field: " + tmp.Field); + } + + s.I.Field = 2; + if (s.I.Field !== 2) { + throw new Error("s.I.Field (1): " + s.I.Field); + } + if (tmp.Field !== 2) { + throw new Error("tmp.Field (1): " + tmp.Field); + } + + s.I = {Field: 3}; // at this point tmp is changed to a copy + if (s.I.Field !== 3) { + throw new Error("s.I.Field (2): " + s.I.Field); + } + if (tmp.Field !== 2) { + throw new Error("tmp.Field (2): " + tmp.Field); + } + `) + + if err != nil { + t.Fatal(err) + } +} diff --git a/object_goslice.go b/object_goslice.go index 4c7a711e..7485466c 100644 --- a/object_goslice.go +++ b/object_goslice.go @@ -15,6 +15,20 @@ type objectGoSlice struct { lengthProp valueProperty } +func (r *Runtime) newObjectGoSlice(data *[]interface{}) *objectGoSlice { + obj := &Object{runtime: r} + a := &objectGoSlice{ + baseObject: baseObject{ + val: obj, + }, + data: data, + } + obj.self = a + a.init() + + return a +} + func (o *objectGoSlice) init() { o.baseObject.init() o.class = classArray @@ -29,11 +43,14 @@ func (o *objectGoSlice) updateLen() { o.lengthProp.value = intToValue(int64(len(*o.data))) } +func (o *objectGoSlice) _getIdx(idx int) Value { + return o.val.runtime.ToValue((*o.data)[idx]) +} + func (o *objectGoSlice) getStr(name unistring.String, receiver Value) Value { var ownProp Value if idx := strToGoIdx(name); idx >= 0 && idx < len(*o.data) { - v := (*o.data)[idx] - ownProp = o.val.runtime.ToValue(v) + ownProp = o._getIdx(idx) } else if name == "length" { ownProp = &o.lengthProp } @@ -43,8 +60,7 @@ func (o *objectGoSlice) getStr(name unistring.String, receiver Value) Value { func (o *objectGoSlice) getIdx(idx valueInt, receiver Value) Value { if idx := int64(idx); idx >= 0 && idx < int64(len(*o.data)) { - v := (*o.data)[idx] - return o.val.runtime.ToValue(v) + return o._getIdx(int(idx)) } if o.prototype != nil { if receiver == nil { @@ -58,9 +74,8 @@ func (o *objectGoSlice) getIdx(idx valueInt, receiver Value) Value { func (o *objectGoSlice) getOwnPropStr(name unistring.String) Value { if idx := strToGoIdx(name); idx >= 0 { if idx < len(*o.data) { - v := o.val.runtime.ToValue((*o.data)[idx]) return &valueProperty{ - value: v, + value: o._getIdx(idx), writable: true, enumerable: true, } @@ -75,9 +90,8 @@ func (o *objectGoSlice) getOwnPropStr(name unistring.String) Value { func (o *objectGoSlice) getOwnPropIdx(idx valueInt) Value { if idx := int64(idx); idx >= 0 && idx < int64(len(*o.data)) { - v := o.val.runtime.ToValue((*o.data)[idx]) return &valueProperty{ - value: v, + value: o._getIdx(int(idx)), writable: true, enumerable: true, } @@ -311,20 +325,26 @@ func (o *objectGoSlice) equal(other objectImpl) bool { return false } -func (o *objectGoSlice) sortLen() int64 { - return int64(len(*o.data)) +func (o *objectGoSlice) esValue() Value { + return o.val } -func (o *objectGoSlice) sortGet(i int64) Value { - return o.getIdx(valueInt(i), nil) +func (o *objectGoSlice) reflectValue() reflect.Value { + return reflect.ValueOf(o.data).Elem() } -func (o *objectGoSlice) swap(i, j int64) { - ii := valueInt(i) - jj := valueInt(j) - x := o.getIdx(ii, nil) - y := o.getIdx(jj, nil) +func (o *objectGoSlice) setReflectValue(value reflect.Value) { + o.data = value.Addr().Interface().(*[]interface{}) +} + +func (o *objectGoSlice) sortLen() int { + return len(*o.data) +} + +func (o *objectGoSlice) sortGet(i int) Value { + return o.getIdx(valueInt(i), nil) +} - o.setOwnIdx(ii, y, false) - o.setOwnIdx(jj, x, false) +func (o *objectGoSlice) swap(i int, j int) { + (*o.data)[i], (*o.data)[j] = (*o.data)[j], (*o.data)[i] } diff --git a/object_goslice_reflect.go b/object_goslice_reflect.go index 2da5f8dc..8ebbb074 100644 --- a/object_goslice_reflect.go +++ b/object_goslice_reflect.go @@ -31,6 +31,15 @@ func (o *objectGoSliceReflect) grow(size int) { n := reflect.MakeSlice(o.value.Type(), size, growCap(size, o.value.Len(), oldcap)) reflect.Copy(n, o.value) o.value.Set(n) + l := len(o.valueCache) + if l > size { + l = size + } + for i, w := range o.valueCache[:l] { + if w != nil { + w.setReflectValue(o.value.Index(i)) + } + } } else { tail := o.value.Slice(o.value.Len(), size) zero := reflect.Zero(o.value.Type().Elem()) @@ -43,6 +52,7 @@ func (o *objectGoSliceReflect) grow(size int) { } func (o *objectGoSliceReflect) shrink(size int) { + o.valueCache.shrink(size) tail := o.value.Slice(size, o.value.Len()) zero := reflect.Zero(o.value.Type().Elem()) for i := 0; i < tail.Len(); i++ { @@ -79,10 +89,3 @@ func (o *objectGoSliceReflect) defineOwnPropertyStr(name unistring.String, descr } return o.objectGoArrayReflect.defineOwnPropertyStr(name, descr, throw) } - -func (o *objectGoSliceReflect) equal(other objectImpl) bool { - if other, ok := other.(*objectGoSliceReflect); ok { - return o.value.Interface() == other.value.Interface() - } - return false -} diff --git a/object_goslice_reflect_test.go b/object_goslice_reflect_test.go index 150347ec..fc5d241a 100644 --- a/object_goslice_reflect_test.go +++ b/object_goslice_reflect_test.go @@ -1,6 +1,7 @@ package goja import ( + "reflect" "testing" ) @@ -439,6 +440,50 @@ func TestGoSliceReflectExportAfterGrow(t *testing.T) { } } +func TestGoSliceReflectSort(t *testing.T) { + vm := New() + type Thing struct{ Name string } + vm.Set("v", []*Thing{ + {Name: "log"}, + {Name: "etc"}, + {Name: "test"}, + {Name: "bin"}, + }) + ret, err := vm.RunString(` +//v.sort((a, b) => a.Name.localeCompare(b.Name)).map((x) => x.Name); + const tmp = v[0]; + v[0] = v[1]; + v[1] = tmp; + v[0].Name + v[1].Name; +`) + if err != nil { + panic(err) + } + t.Log(ret.Export()) +} + +func TestGoSliceReflect111(t *testing.T) { + vm := New() + vm.Set("v", []int32{ + 1, 2, + }) + ret, err := vm.RunString(` +//v.sort((a, b) => a.Name.localeCompare(b.Name)).map((x) => x.Name); + const tmp = v[0]; + v[0] = v[1]; + v[1] = tmp; + "" + v[0] + v[1]; +`) + if err != nil { + panic(err) + } + t.Log(ret.Export()) + a := []int{1, 2} + a0 := reflect.ValueOf(a).Index(0) + a0.Set(reflect.ValueOf(0)) + t.Log(a[0]) +} + func BenchmarkGoSliceReflectSet(b *testing.B) { vm := New() a := vm.ToValue([]int{1}).(*Object) diff --git a/object_goslice_test.go b/object_goslice_test.go index fd968298..66651028 100644 --- a/object_goslice_test.go +++ b/object_goslice_test.go @@ -242,3 +242,19 @@ func TestGoSliceLengthProperty(t *testing.T) { t.Fatal(err) } } + +func TestGoSliceSort(t *testing.T) { + vm := New() + s := []interface{}{4, 2, 3} + vm.Set("s", &s) + _, err := vm.RunString(`s.sort()`) + if err != nil { + t.Fatal(err) + } + if len(s) != 3 { + t.Fatalf("len: %d", len(s)) + } + if s[0] != 2 || s[1] != 3 || s[2] != 4 { + t.Fatalf("val: %v", s) + } +} diff --git a/object_lazy.go b/object_lazy.go index e8de40e2..ea427199 100644 --- a/object_lazy.go +++ b/object_lazy.go @@ -295,19 +295,19 @@ func (o *lazyObject) setProto(proto *Object, throw bool) bool { return obj.setProto(proto, throw) } -func (o *lazyObject) sortLen() int64 { +func (o *lazyObject) sortLen() int { obj := o.create(o.val) o.val.self = obj return obj.sortLen() } -func (o *lazyObject) sortGet(i int64) Value { +func (o *lazyObject) sortGet(i int) Value { obj := o.create(o.val) o.val.self = obj return obj.sortGet(i) } -func (o *lazyObject) swap(i, j int64) { +func (o *lazyObject) swap(i int, j int) { obj := o.create(o.val) o.val.self = obj obj.swap(i, j) diff --git a/runtime.go b/runtime.go index 5ba15ba0..7e816505 100644 --- a/runtime.go +++ b/runtime.go @@ -1444,19 +1444,10 @@ func (r *Runtime) ClearInterrupt() { ToValue converts a Go value into a JavaScript value of a most appropriate type. Structural types (such as structs, maps and slices) are wrapped so that changes are reflected on the original value which can be retrieved using Value.Export(). -WARNING! There are two very important caveats to bear in mind when modifying wrapped Go structs, maps and -slices. +WARNING! These wrapped Go values do not behave in the same way as native ECMAScript values. If you plan to modify +them in ECMAScript, bear in mind the following caveats: -1. If a slice is passed by value (not as a pointer), resizing the slice does not reflect on the original -value. Moreover, extending the slice may result in the underlying array being re-allocated and copied. -For example: - - a := []interface{}{1} - vm.Set("a", a) - vm.RunString(`a.push(2); a[0] = 0;`) - fmt.Println(a[0]) // prints "1" - -2. If a regular JavaScript Object is assigned as an element of a wrapped Go struct, map or array, it is +1. If a regular JavaScript Object is assigned as an element of a wrapped Go struct, map or array, it is Export()'ed and therefore copied. This may result in an unexpected behaviour in JavaScript: m := map[string]interface{}{} @@ -1468,7 +1459,74 @@ Export()'ed and therefore copied. This may result in an unexpected behaviour in `) fmt.Println(m["obj"].(map[string]interface{})["test"]) // prints "false" -Non-addressable structs, slices and arrays get copied (as if they were passed as a function parameter, by value). +2. Be careful with nested non-pointer compound types (structs, slices and arrays) if you modify them in +ECMAScript. Better avoid it at all if possible. One of the fundamental differences between ECMAScript and Go is in +the former all Objects are references whereas in Go you can have a literal struct or array. Consider the following +example: + + type S struct { + Field int + } + + a := []S{{1}, {2}} // slice of literal structs + vm.Set("a", &a) + vm.RunString(` + let tmp = {Field: 1}; + a[0] = tmp; + a[1] = tmp; + tmp.Field = 2; + `) + +In ECMAScript one would expect a[0].Field and a[1].Field to be equal to 2, but this is really not possible +(or at least non-trivial without some complex reference tracking). + +To cover the most common use cases and to avoid excessive memory allocation, the following 'copy-on-change' mechanism +is implemented (for both arrays and structs): + +* When a nested compound value is accessed, the returned ES value becomes a reference to the literal value. +This ensures that things like 'a[0].Field = 1' work as expected and simple access to 'a[0].Field' does not result +in copying of a[0]. + +* The original container ('a' in our case) keeps track of the returned reference value and if a[0] is reassigned +(e.g. by direct assignment, deletion or shrinking the array) the old a[0] is copied and the earlier returned value +becomes a reference to the copy: + + let tmp = a[0]; // no copy, tmp is a reference to a[0] + tmp.Field = 1; // a[0].Field === 1 after this + a[0] = {Field: 2}; // tmp is now a reference to a copy of the old value (with Field === 1) + a[0].Field === 2 && tmp.Field === 1; // true + +* Array value swaps caused by in-place sort (using Array.prototype.sort()) do not count as re-assignments, instead +the references are adjusted to point to the new indices. + +* Assignment to an inner compound value always does a copy (and sometimes type conversion): + + a[1] = tmp; // a[1] is now a copy of tmp + tmp.Field = 3; // does not affect a[1].Field + +3. Non-addressable structs, slices and arrays get copied. This sometimes may lead to a confusion as assigning to +inner fields does not appear to work: + + a1 := []interface{}{S{1}, S{2}} + vm.Set("a1", &a1) + vm.RunString(` + a1[0].Field === 1; // true + a1[0].Field = 2; + a1[0].Field === 2; // FALSE, because what it really did was copy a1[0] set its Field to 2 and immediately drop it + `) + +An alternative would be making a1[0].Field a non-writable property which would probably be more in line with +ECMAScript, however it would require to manually copy the value if it does need to be modified which may be +impractical. + +Note, the same applies to slices. If a slice is passed by value (not as a pointer), resizing the slice does not reflect on the original +value. Moreover, extending the slice may result in the underlying array being re-allocated and copied. +For example: + + a := []interface{}{1} + vm.Set("a", a) + vm.RunString(`a.push(2); a[0] = 0;`) + fmt.Println(a[0]) // prints "1" Notes on individual types: @@ -1629,10 +1687,6 @@ Note that the underlying type is not lost, calling Export() returns the original reflect based types. */ func (r *Runtime) ToValue(i interface{}) Value { - return r.toValue(i, reflect.Value{}) -} - -func (r *Runtime) toValue(i interface{}, origValue reflect.Value) Value { switch i := i.(type) { case nil: return _null @@ -1722,44 +1776,18 @@ func (r *Runtime) toValue(i interface{}, origValue reflect.Value) Value { if i == nil { return _null } - obj := &Object{runtime: r} - a := &objectGoSlice{ - baseObject: baseObject{ - val: obj, - }, - data: &i, - } - obj.self = a - a.init() - return obj + return r.newObjectGoSlice(&i).val case *[]interface{}: if i == nil { return _null } - obj := &Object{runtime: r} - a := &objectGoSlice{ - baseObject: baseObject{ - val: obj, - }, - data: i, - } - obj.self = a - a.init() - return obj + return r.newObjectGoSlice(i).val } - if !origValue.IsValid() { - origValue = reflect.ValueOf(i) - } else { - // If origValue was a result of an Index(), or Field(), or such, its Kind may be Interface: - // a := []interface{}{(*S)(nil)} - // a0 := reflect.ValueOf(a).Index(0) // a0.Kind() is reflect.Interface - // a1 := reflect.ValueOf(a[0]) // a1.Kind() is reflect.Ptr - // Need to "dereference" it to make it consistent with plain value being passed. - for origValue.Kind() == reflect.Interface { - origValue = origValue.Elem() - } - } + return r.reflectValueToValue(reflect.ValueOf(i)) +} + +func (r *Runtime) reflectValueToValue(origValue reflect.Value) Value { value := origValue for value.Kind() == reflect.Ptr { value = reflect.Indirect(value) @@ -1824,7 +1852,7 @@ func (r *Runtime) toValue(i interface{}, origValue reflect.Value) Value { obj.self = a return obj case reflect.Func: - name := unistring.NewFromString(runtime.FuncForPC(reflect.ValueOf(i).Pointer()).Name()) + name := unistring.NewFromString(runtime.FuncForPC(value.Pointer()).Name()) return r.newNativeFunc(r.wrapReflectFunc(value), nil, name, nil, value.Type().NumIn()) }