Using onheap keyword in concurrent classes #1714
Conversation
| @@ -109,7 +109,7 @@ Future: abstract class <T> { | |||
| } | |||
|
|
|||
| _ThreadFuture: class <T> extends Future<T> { | |||
| _result: Object | |||
| _result: __onheap__ T | |||
There was a problem hiding this comment.
And just when I thought ooc code can't get more confusing than it already is... Is inline assembly the next step ? :P
There was a problem hiding this comment.
This is actually exactly the keyword that Cell uses to make sure its contents doesn't disappear in a long lost stack frame.
We should really change the keyword to something prettier, though.
There was a problem hiding this comment.
keyword that Cell uses
Yeah, and it's slowly making it's way to higher level classes :)
There was a problem hiding this comment.
I've pointed out on rock#48 that this could be automatically done by the compiler, would be happy to oblige if you think it's worth it now that it isn't used in many places.
(So I can remove the keyword from the language too at the same time now that it's only used in a couple of places)
There was a problem hiding this comment.
Nice, I hadn't seen that. I would certainly prefer to not have to manually add this keyword, but I will need to look into it some more. @thomasfanell and @fredrikbryntesson , thoughts?
There was a problem hiding this comment.
Thanks, I think we should be less explicit about memory management if we can.
I'd love to see this done automatically, but I think @thomasfanell or @fredrikbryntesson should be the one to decide.
marcusnaslund
force-pushed
the
concurrentonheap
branch
2 times, most recently
from
59b0724 to
83dec62
Compare
| @@ -65,6 +65,8 @@ ThreadPoolTest: class extends Fixture { | |||
| comparison := t"fail" | |||
| result := future wait(comparison) | |||
| expect(result == comparison) | |||
| Time sleepMilli(1000) | |||
There was a problem hiding this comment.
We can currently not wait on a Future we have previously called cancel on. This is ugly but works for now. We will get something better with #1588 or similar.
|
Obviously, if we could have rock generate the equivalent of |
|
Peer review @sebastianbaginski ? |
ghost
added
|
👍 Will be doing some stress tests before I merge this. |
marcusnaslund
force-pushed
the
concurrentonheap
branch
from
83dec62 to
7abc70d
Compare
marcusnaslund
force-pushed
the
concurrentonheap
branch
from
7abc70d to
eb87d56
Compare
|
Works like a charm. |
This is better than creating a new
Cellinstance every time (which also leaks in certain circumstances, as memchecking the tests show).This PR fixes all (yay!) memory leaks in concurrent tests as part of #1699 together with #1712
Will have to be tested thoroughly.