SI-8946: Fixes memory leak when using reflection #4148
Conversation
|
Not sure why this failed after rebase; should've been harmless. Investigating. |
|
I'd forgotten to build against JDK6 locally :( My bad. I'll get this fixed soon. |
timcharper
force-pushed
the
issue/SI-8946
branch
from
db95d06 to
ed8589a
Compare
|
this should build against JDK6: |
References to Threads would be retained long after their termination if reflection is used in them. This led to a steady, long memory leak in applications using reflection in thread pools.
timcharper
force-pushed
the
issue/SI-8946
branch
from
ed8589a to
d367c5f
Compare
timcharper
force-pushed
the
issue/SI-8946
branch
from
d367c5f to
6cb1add
Compare
timcharper
force-pushed
the
issue/SI-8946
branch
from
6cb1add to
d367c5f
Compare
timcharper
changed the title
|
Thanks! |
|
LGTM. @retronym WDYT? |
|
I had to look at this a few times to convince myself that it is okay. But I can now see that the fact we're running on thread which is the key to the map prevents usual problems with concurrent map mutation and with a weak reference being collected between the Thanks again, @timcharper! |
SI-8946: Fixes memory leak when using reflection
|
Unfortunately the assert is failing on jenkins (under load, likely): |
|
I haven't looked at this failure, but I have a different approach at to checking that a ref is collectable. There is no guarantee that refs are cleared in a particular order, so it's safer to wait on the ref queue. In fact, the allocator could check the ref and short-circuit the wait: the docs say the ref is enqueued maybe later, after the ref is cleared. I added a collectable assertion, which maybe should use the queue instead of the optimistic approach used here. |
|
Som-snytt - interesting approach. I'll review and incorporate your ideas, and will send another patch here soon (tonight). Sorry@adriaanm for the trouble! |
|
@som-snytt it looks like you are doing a very similar approach to mine for checking collectability; and, it looks like the tests in your examples may be prone to the same error as I am seeing. I think a retry mechanism will work best. Also, if after a set amount of retries, one or more threads are garbage collector, I think victory can be declared |
|
Please see the following patch @som-snytt @adriaanm SpinGo@286538b |
|
I like your idea of giving the gc many references and only caring if one is cleared, since that shows that they are all collectable in principle. In fact, given a generator of a (new) reference, the test rig can invoke the generator any number of times, which can only increase our chances of noticing a collection. I used your strategy (of checking for cleared ref after an arbitrary ref was cleared) in the first commit here: but the second commit uses that other strategy of just blocking on the queue while a thread generates garbage. The thread stops generating garbage if the heap expands or the reference is cleared. The docs say that when a ref is cleared, enqueuing happens at the same time or later. So there is room for further improvement: if the allocator notices the ref is cleared, it should notify the test thread right away rather than let it wait for enqueuing. But that is very refined. I tried to avoid arbitrary sleeps and retries, since tuning that is hard. Tuning a timeout is fairly simple, though it suffers from long waits for failure. Eventually, I'd want a long wait for Jenkins and a short wait when I run unit tests locally. |
|
I would suggest moving the test case from Then you can eat your fill of turkey before reasoning about garbage collection as dessert. |
|
There's a test at The tests pass, but maybe I'll ask Jenkins nicely to retry a few times, just to confirm. |
In the spirit of scala/scala#4148, which fixed SI-8731. The consensus seems to be that the intent of the @switch annotation is to warn when the generated bytecode may be poor, not merely if the compiler elects to not emit a tableswitch/lookupswitch. Note that the case threshold for determining whether a switch is emitted is implementation dependent, and currently varies between scalac and dotc. Also note that this implementation will not warn in instances where a switch will never be emitted (e.g. because the match is on a non-integral type) but the number of cases is below the warning threshold. This behavior is consistent with scalac, but may be surprising to the user if another case is added later and a warning suddenly appears. Not all spurious @switch warnings are addressed by this commit, see issue scala#5070 for an example.
References to Threads would be retained long after their termination if
reflection is used in them. This led to a steady, long memory leak in
applications using reflection in thread pools.
Closes https://issues.scala-lang.org/browse/SI-8946
Mentioning @retronym @xeno-by