Workaround memory leak in dispatchers. #119
Conversation
Android has a long-standing known issue where local variables aren't explicitly cleared even when they go out of scope, which can cause their contents to leak. Since BlockingQueue#take() blocks forever until a new item is ready, this means the last request will remain in memory until a new request pushes it out. Extracting a helper method is a workaround for this - see, for example, the following CL in the Android support lib: https://android.googlesource.com/platform/frameworks/support/+/cd07a0cfd9c9501a03c574d2d48df51c82b73e33 The following other solutions were attempted but were not sufficient: - Clear the variable prior to take() - optimized out because the write is not observable, so it has no impact on the bytecode. - Call poll() prior to take() - for some reason, this doesn't work when proguard optimization is on. With code optimization, there's no guarantee that this will work. However, it appears to be the best we can do and follows precedent / advice from the ART team. Should contain no functional changes otherwise as this is just extracting code to a helper method, and thus should be safe for 1.1.0. Verified against provided sample app. Fixes google#114
|
OK - I'm reasonably confident this is the best we can do. PTAL - thanks :) |
|
I glanced at the diffs and nothing pops out. This compiler behavior violates JLS. I suggest marking the workarounds with the compiler bug's URL for tracking, so the workaround can be removed eventually. Something like this existed at one time, but was wiped out years ago. I think this current issue is something new, and will be wiped out eventually. |
|
From what I've gathered internally it's not actually a violation of JLS, which doesn't specify anything about when the variable in question must become inaccessible. It's not a correctness issue, only an issue with the fact that the object which is out of scope might be heavy and thus should be more aggressively pruned. So it becomes a question of whether something further down the line can make the needed optimization here, and doing so safely in all circumstances is challenging. The bugs tracking this are internal to Google, but for our reference, the primary bug number is 33158143. |
|
One other thing I'd like to look into before merging would be adding a consumerProguardFile with rules to "-keep,allowshrinking,allowobfuscation" both of the methods that we don't want to inline. That should offer a stronger guarantee of no inlining, even if it seems like the problem doesn't manifest with proguard on. |
Android has a long-standing known issue where local variables aren't
explicitly cleared even when they go out of scope, which can cause
their contents to leak. Since BlockingQueue#take() blocks
forever until a new item is ready, this means the last
request will remain in memory until a new request pushes it
out. Extracting a helper method is a workaround for this - see, for
example, the following CL in the Android support lib:
https://android.googlesource.com/platform/frameworks/support/+/cd07a0cfd9c9501a03c574d2d48df51c82b73e33
The following other solutions were attempted but were not sufficient:
Clear the variable prior to take() - optimized out because the write
is not observable, so it has no impact on the bytecode.
Call poll() prior to take() - for some reason, this doesn't work
when proguard optimization is on.
With code optimization, there's no guarantee that this will work.
However, it appears to be the best we can do and follows precedent /
advice from the ART team.
Should contain no functional changes otherwise as this is just
extracting code to a helper method, and thus should be safe for 1.1.0.
Verified against provided sample app.
Fixes #114