Skip to content
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

Optimizer removes assignment too eagerly => memory leaks? #5313

Closed
scabug opened this Issue Dec 14, 2011 · 10 comments

Comments

Projects
None yet
1 participant
@scabug
Copy link

scabug commented Dec 14, 2011

The null assignment in the following snippet is removed when compiled with -optimise which leads to different behavior and can result in memory leaks. It seems to be a bit of an edge case, since obj is a local var, but it is surprising nonetheless. I actually encountered this in a unit test testing for leaks, where a setup like below can be common.

object WeakBug {
  def main(args: Array[String]) {
    var obj = new Object {}
    val ref = new java.lang.ref.WeakReference(obj)
    obj = null
    System.gc()
    //Thread.sleep(500)
    assert(ref.get == null)
    println(ref.get)
  }
}
@scabug

This comment has been minimized.

Copy link
Author

scabug commented Dec 14, 2011

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 21, 2012

@VladUreche said:
After we'll have GenBCode running, I'll have another look at this specific bug. Until then, it's just fixing a piece of code that'll soon get replaced -- not the best use of time.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 23, 2013

@JamesIry said:
Basically I don't think we can eliminate any local stores to reference types because of gc and/or weak/phantom observability. For instance

def foo = {
  var x = new HumungoObject
  // use x
  x = new SmallObject
  blah()
}

The reassignment to x frees up the reference to the HumungoObject, making it elligible for gc and freeing up memory for blah() to work.

Maybe there are some heuristics we could use like eliminating a local store that's the last instruction of a method before a return or some such. But whatever we come up with is bound to catch relatively few cases and probably won't be worth the effort.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 24, 2013

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 25, 2013

@JamesIry said:
That had, um, unacceptable consequences. Because I marked all ref type stores as necessary we were seeing them as uses of closures. So inlined closures weren't being eliminated even though the inlining eliminated all actual use of the closure. So my new tentative plan is to deem a store unnecessary if the value isn't used AND (it's not a reference type OR we can prove it doesn't clobber a previous necessary store). That should eliminate a fair number of stores, including unused closures, but should keep the kind of store that destroys an existing reference to a heap object.

Devil. Details.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 25, 2013

@VladUreche said (edited on Jan 25, 2013 9:19:51 PM UTC):
Another approach would be to replace the value in the STORE by null. That would get closure classes eliminated, would keep the side-effect of freeing the reference and would avoid marking the stack slot as necessary. WDYT?

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 25, 2013

@paulp said:
That has to be the winner.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 29, 2013

@JamesIry said:
scala/scala#2001

I have a version that replaces the unneeded store with null. I think I'd rather put that in 2.11 than 2.10.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 29, 2013

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 30, 2013

@JamesIry said:
Changed my mind about the riskiness of the nulling version. It's now in scala/scala#2001.

@scabug scabug closed this Feb 5, 2013

@scabug scabug added this to the 2.10.1 milestone Apr 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.