fix: properly mark array elements when an realm slice is updated #1305
Conversation
deelawn
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1305 +/- ##
==========================================
- Coverage 56.08% 55.85% -0.24%
==========================================
Files 432 431 -1
Lines 65971 65758 -213
==========================================
- Hits 37001 36728 -273
- Misses 26078 26157 +79
+ Partials 2892 2873 -19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
the
📦 ⛰️ gno.land
|
Do you think we can close #960 if this is merged? |
|
Great job on this! FWIW, I'm not sure this is related to #1167 (it's different from #1170). #1167 specifically is about the underlying values of the slice turning to be "zero values", which doesn't seem to be the case here (although the fix may address that too). I can try to check out if the behaviour in GnoChess is fixed with this PR by reverting the commit and trying to do the replication steps for that behaviour (or I can give them to you if you want to try that, I'm going a bit by memory though, don't know if they were written down). |
I think so, yes |
I've made a note, I wanted to do this already but I'll prioritize so we can find out if it's the same issue. Thanks! |
|
So I think we can state that the bugs we have in #960, #1167 and #1170, @deelawn found a great way to overcome that bug, but the bug is still That said, I found a workaround, that could be also applied: when a I think this is a good workaround because its one-line and because we Tell me what you think. |
|
that's the first workaround I attempted, but I had failing tests when I did |
|
@tbruyelle It doesn't seem like something we'd want as a long term solution, but I'm not sure why that might be. Other than the failing tests observed by @n0izn0iz, the first thing that comes to my mind is the question of how this might affect the frequency of growing slices' underlying arrays. But I don't know where that happens so it's just a guess 🤷 In any case, I believe this is close to being merged; it should address all the issues you've listed. Or were you suggesting that there is a separate bug that should be fixed and this PR isn't addressing some underlying root cause? If that's the case then let's have a chat about the nature of the underlying bug and how we can identify where to fix it. |
|
@deelawn I probably spent too much time on this, I thought I had a case where your fix doesn't work but I cannot reproduce. Sorry for that. Anyway for the memo, I think I found a way to reproduce the bug with a single transaction, using a gno file in Here is the content of // PKGPATH: gno.land/r/slice_cap
package slice_cap
var a []int
func init() {
a = make([]int, 0, 1)
}
func main() {
a = append(a, 42)
println(a)
}
// Output:
// slice[(42 int)]
// Realm:
// switchrealm["gno.land/r/slice_cap"]
// u[95eec26514f13645e29d560a2f0866a8dfe44dee:4]={
// "Data": null,
// "List": [
// {
// "N": "KgAAAAAAAAA=",
// "T": {
// "@type": "/gno.PrimitiveType",
// "value": "32"
// }
// }
// ],
// "ObjectInfo": {
// "ID": "95eec26514f13645e29d560a2f0866a8dfe44dee:4",
// "ModTime": "4",
// "OwnerID": "95eec26514f13645e29d560a2f0866a8dfe44dee:2",
// "RefCount": "1"
// }
// }
// u[95eec26514f13645e29d560a2f0866a8dfe44dee:2]={
// "Blank": {},
// "ObjectInfo": {
// "ID": "95eec26514f13645e29d560a2f0866a8dfe44dee:2",
// "IsEscaped": true,
// "ModTime": "4",
// "RefCount": "2"
// },
// "Parent": null,
// "Source": {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "File": "",
// "Line": "0",
// "Nonce": "0",
// "PkgPath": "gno.land/r/slice_cap"
// }
// },
// "Values": [
// {
// "T": {
// "@type": "/gno.FuncType",
// "Params": [],
// "Results": []
// },
// "V": {
// "@type": "/gno.FuncValue",
// "Closure": {
// "@type": "/gno.RefValue",
// "Escaped": true,
// "ObjectID": "95eec26514f13645e29d560a2f0866a8dfe44dee:3"
// },
// "FileName": "main.gno",
// "IsMethod": false,
// "Name": "init.0",
// "PkgPath": "gno.land/r/slice_cap",
// "Source": {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "File": "main.gno",
// "Line": "6",
// "Nonce": "0",
// "PkgPath": "gno.land/r/slice_cap"
// }
// },
// "Type": {
// "@type": "/gno.FuncType",
// "Params": [],
// "Results": []
// }
// }
// },
// {
// "T": {
// "@type": "/gno.FuncType",
// "Params": [],
// "Results": []
// },
// "V": {
// "@type": "/gno.FuncValue",
// "Closure": {
// "@type": "/gno.RefValue",
// "Escaped": true,
// "ObjectID": "95eec26514f13645e29d560a2f0866a8dfe44dee:3"
// },
// "FileName": "main.gno",
// "IsMethod": false,
// "Name": "main",
// "PkgPath": "gno.land/r/slice_cap",
// "Source": {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "File": "main.gno",
// "Line": "10",
// "Nonce": "0",
// "PkgPath": "gno.land/r/slice_cap"
// }
// },
// "Type": {
// "@type": "/gno.FuncType",
// "Params": [],
// "Results": []
// }
// }
// },
// {
// "T": {
// "@type": "/gno.SliceType",
// "Elt": {
// "@type": "/gno.PrimitiveType",
// "value": "32"
// },
// "Vrd": false
// },
// "V": {
// "@type": "/gno.SliceValue",
// "Base": {
// "@type": "/gno.RefValue",
// "Hash": "4ba43144bcf3cb8df3f5b60efaa9e30e1232b468",
// "ObjectID": "95eec26514f13645e29d560a2f0866a8dfe44dee:4"
// },
// "Length": "1",
// "Maxcap": "1",
// "Offset": "0"
// }
// }
// ]
// }Using the
So this version works on this branch, while it fails on |
|
@thehowl added a test case to #1167 that this PR does not address. I'm going to unlink that issue from this PR and will address it in a subsequent PR. I've been discussing with @piux2 and am trying to figure out a way to rework the part of this PR that is concerned with marking appended slice elements as "new created" when the append does not result in the resizing of the underlying array. |
|
This looks good to me and the fix works well. Two issues got fixed in this PR
@deelawn and I looked into the options to fix the second problem at the append() in uverse.go, and breaking down to two statements in preprocess stage. We have not found better solutions than the current one, which may have more efficient alternatives. maybe @jaekwon can take a look as well. |
|
looking at this now. please wait for my approval to merge. |
|
Maybe append() in uverse isn't property setting the array as dirty by calling DidUpdate. NOTE: The design consideration for DidUpdate is that it must be fast. Here is a stab at suggested changes to append() (while reverting to original DidUpdate): diff --git a/gnovm/pkg/gnolang/uverse.go b/gnovm/pkg/gnolang/uverse.go
index 57f8f6d3..de22a29a 100644
--- a/gnovm/pkg/gnolang/uverse.go
+++ b/gnovm/pkg/gnolang/uverse.go
@@ -294,14 +294,23 @@ func UverseNode() *PackageNode {
// append(*SliceValue.List, *SliceValue) ---------
list := xvb.List
if argsb.Data == nil {
- copy(
- list[xvo+xvl:xvo+xvl+argsl],
- argsb.List[argso:argso+argsl])
+ // XXX find other uses of native copy()
+ // and replace with .Copy(alloc) as necessary.
+ for i := 0; i < argsl; i++ {
+ oldelem := list[xvo+xvl+i]
+ newelem := argsb.List[argso+i].Copy(m.Alloc)
+ list[xvo+xvl+i] = newelem
+ m.Realm.DidUpdate(xvb,
+ oldelem.GetFirstObject(m.Store),
+ newelem.GetFirstObject(m.Store),
+ )
+ }
} else {
copyDataToList(
list[xvo+xvl:xvo+xvl+argsl],
argsb.Data[argso:argso+argsl],
xt.Elem())
+ m.Realm.DidUpdate(xvb, nil, nil)
}
} else {
// append(*SliceValue.Data, *SliceValue) ---------
@@ -315,6 +324,7 @@ func UverseNode() *PackageNode {
data[xvo+xvl:xvo+xvl+argsl],
argsb.Data[argso:argso+argsl])
}
+ m.Realm.DidUpdate(xvb, nil, nil)
}
m.PushValue(TypedValue{
T: xt,
@@ -363,9 +373,9 @@ func UverseNode() *PackageNode {
list := make([]TypedValue, xvl+argsl)
if 0 < xvl {
if xvb.Data == nil {
- copy(
- list[:xvl],
- xvb.List[xvo:xvo+xvl])
+ for i := 0; i < xvl; i++ {
+ list[i] = xvb.List[xvo+i].Copy(m.Alloc)
+ }
} else {
panic("should not happen")
/*
@@ -379,9 +389,9 @@ func UverseNode() *PackageNode {
}
if 0 < argsl {
if argsb.Data == nil {
- copy(
- list[xvl:xvl+argsl],
- argsb.List[argso:argso+argsl])
+ for i := 0; i < argsl; i++ {
+ list[xvl+i] = argsb.List[argso+i].Copy(m.Alloc)
+ }
} else {
copyDataToList(
list[xvl:xvl+argsl], |
|
Yeah the above patch seems to fix the problem for test 1167. 
DidUpdate and FinalizeRealmTransaction is among the trickiest logic btw, so please definitely include me in the review process! Thanks for the heads up. |
|
looking |
| switch tv.V.(type) { | ||
| case RefValue: | ||
| cp.T = tv.T | ||
| refObject := tv.GetFirstObject(store) |
There was a problem hiding this comment.
can't we do store.GetObject(cv.ObjectID) directly here?
There was a problem hiding this comment.
to me it seems a little strange to uise GetFirstObject outside of a realm-persistence context.
Maybe s/GetFirstObject/getFirstObject/g.
gnovm/pkg/gnolang/values.go
Outdated
| @@ -1010,6 +1010,28 @@ func (tv TypedValue) Copy(alloc *Allocator) (cp TypedValue) { | |||
| return | |||
| } | |||
|
|
|||
| // DeepCopy makes of copy of the underlying value in the case of reference values. | |||
There was a problem hiding this comment.
This is a misnomer. It doesn't do a deep copy, a deep copy should deep copy all children recursively. It only does a slightly deeper copy if tv happens to be a RefValue. But this function is useful at least in the context of uverse, so I would call this "unrefCopy" and not even expose it.
There was a problem hiding this comment.
"// unrefCopy makes a copy of the underlying value, but first unreferences if RefValue."
|
What about this part? // XXX find other uses of native copy() Now we have to look at the codebase and see if there are in the very least other uverse functions that need the same fixing... they all seem to use Assign2() which calls DidUpdate already, which is good. Any other places besides uverse? I don't think so. |
This reverts commit 91b4586.
jaekwon
force-pushed
the
bug/slice-append
branch
from
5e2948a
to
5425504
Compare
…lang#1305) Addresses gnolang#1167, gnolang#960, and gnolang#1170 Consider the following situation: - A slice of structs exists with a length of zero and a capacity of one - A new struct literal is appended to the slice - The code panics because the newly allocated struct literal was never marked as "new" ``` go package append import ( "gno.land/p/demo/ufmt" ) type T struct{ i int } var a []T func init() { a = make([]T, 0, 1) } func Append(i int) { a = append(a, T{i: i}) } ``` Invoking the `Append` function will cause a panic. The solution is to traverse each of the array elements after slice append assignment to make sure any new or updated elements are marked as such. This PR also includes a change to ensure that marking an object as dirty and managing references to the object are mutually exclusive. I think this is correct but am not sure. The changes include txtar test cases that incorporate the issue described by @tbruyelle in gnolang#1170 --------- Co-authored-by: jaekwon <[email protected]>
Addresses #1167, #960, and #1170
Consider the following situation:
Invoking the
Appendfunction will cause a panic.The solution is to traverse each of the array elements after slice append assignment to make sure any new or updated elements are marked as such.
This PR also includes a change to ensure that marking an object as dirty and managing references to the object are mutually exclusive. I think this is correct but am not sure.
The changes include txtar test cases that incorporate the issue described by @tbruyelle in #1170