-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Discussion] Do we need =sink
operator at all?
#13003
Comments
I have one example that needs discussion, I don't it works well now either: type MyObject = object
len: int
data: UncheckedArray[int] Replacing Update: not a problem. |
But There is one case where it can be better though: Instead of destroy+copyMem+wasMoved you can treat =sink as an ordinary assignment and reuse an internal buffer, saving the destroy. I think... |
This idea was discussed a few times on IRC. |
I don't know about the For example you could do the following overload to avoid an extra buffer proc map[T](s: seq[T], p: proc[T](elem: T): T): seq[T] =
result = newSeq[T](s.len)
for i in 0 .. s.len:
result[i] = p(s[i])
proc map[T](s: sink seq[T], p: proc[T](elem: T): T): seq[T] =
for i in 0 .. s.len:
s[i] = p(s[i])
`=sink`(result, s) Also as mentioned in #12631, for handling atomics users need to be able to override assignment/destructors operators with threadsafe versions. |
@mratsim: sink type is clearly useful, clears stays. I need to think about atomics. @Clyybber: I got your point. The Looks like we moving towards a destructor spec update. So far includes everything proposed to date: `=destroy`(dest: var T) # no changes here, user implemented destroy if not empty
`=`(dest: var T, src: T, isDestEmpty: bool) # deepcopy into dest from src,
# if isDestEmpty is true it is safe not to destroy `dest`.
# if isDestEmpty is false it is important not to use any dest fieds as they can be not initialized
`=sink`(dest: var T, src: T, isDestEmpty: bool) # now optional operator, compiler will use memcopy if not provided.
# default compiler implementation is efficient, hence user should provide one only at his own risk. Let me know your thoughts? |
@cooldome I think if |
@Clyybber: static bool might be not bad idea, need to check how easy it is to implement though. |
@cooldome If |
After more thought I am leaning towards more simple and straight forward proposal: `=destroy`(dest: var T) # no changes here, user implemented destroy if not empty
`=`(dest: var T, src: T) # deepcopy into dest from src, dest is always empty.
`=sink`(dest: var T, src: T) # now optional operator, compiler will use memcopy if not provided.
# dest is always empty. This proposal is closer to what we currently have and easier to implement. In Clyybber's terms |
Overloading based on |
@Araq: could you please explain your comment a bit more I am not sure how operator overloading came into play. I suggested to make Example: let x = a will be compiled to var x # with sfNoInit flag
`=`(x, a) instead of var x
x = default()
`=`(x, y) # inside `=` there is `=destroy`(x) call One more example: x = a will be compiled to: `=destroy`(x)
`=`(x, a) instead of: `=`(x, a) # inside `=` there is `=destroy`(x) call What do think about this idea? |
Yeah, I agree completely, it's what we observed too, @Clyybber is also working on this. :-) I summarized it as "optimize init+sink to copyMem" on the forum. There is also "optimize away wasMoved+destructor call".
This is dangerous for performance! Inside |
Hi Gents, First proposal, mild: proc sink(dest: var T, src: T) =
`=destroy(dest)`
dest = src # dest is cursor here, hence it is a mem copy operation This implementation is efficient so mention in the docs that sink operator is now optional. Second proposal more radical: proc genSink(c: var Con; dest, ri: PNode): PNode =
if isFirstWrite(dest, c): # optimize sink call into a bitwise memcopy
result = newTree(nkFastAsgn, dest)
else:
result = # not quite correct but you got the idea
genDestroy(dest)
newTree(nkFastAsgn, dest) Remove Benefits: shaves off large number of generated procs. Less hooks user needs to provide the better. |
I completely agree with the philosphy and direction but I'm also afraid we're losing something. Consider @mratsim's remark that |
So my conclusion. First mild proposal is OK, second one let's wait and see if people are going to find alternative ways to use sink hook. |
Discussion is closed. Mild proposal was implemented. I will keep issue open to see if we need to implement stronger proposal after it gets clearer if custom sink operator would be useful for atomics. |
While working on #13002 I had this weird idea that I want to share. It is possible that we don't need
=sink
operator at all. It doesn't mean we will not move objects. I suggest we will replace=sink
with the combination of the following three operations:Therefore
=sink
will always mean bitwise shallow copy. In old days sink operator contained an if statement that checked if it is self assignment. It is now gone it is done outside. Compiler also takes the liberty of replacing=sink
calls with bitwise copy when it likes it. Possibly we can just simplify the stack and do bitwise copy in all cases.It is easy to prove that I am wrong just provide one counter example that will not work without
=sink
operator.The text was updated successfully, but these errors were encountered: