Scala Programming Language
  1. Scala Programming Language
  2. SI-5313

Optimizer removes assignment too eagerly => memory leaks?

    Details

      Description

      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)
        }
      }
      

        Activity

        Hide
        Vlad Ureche added a comment -

        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.

        Show
        Vlad Ureche added a comment - 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.
        Hide
        James Iry added a comment -

        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.

        Show
        James Iry added a comment - 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.
        Show
        James Iry added a comment - https://github.com/scala/scala/pull/1959
        Hide
        James Iry added a comment -

        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.

        Show
        James Iry added a comment - 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.
        Hide
        Vlad Ureche added a comment - - edited

        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?

        Show
        Vlad Ureche added a comment - - edited 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?
        Hide
        Paul Phillips added a comment -

        That has to be the winner.

        Show
        Paul Phillips added a comment - That has to be the winner.
        Hide
        James Iry added a comment -

        https://github.com/scala/scala/pull/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.

        Show
        James Iry added a comment - https://github.com/scala/scala/pull/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.
        Hide
        James Iry added a comment -
        Show
        James Iry added a comment - The nulling version looks like this https://github.com/JamesIry/scala/commits/wip_2.10.x_SI-5313_2
        Hide
        James Iry added a comment -

        Changed my mind about the riskiness of the nulling version. It's now in https://github.com/scala/scala/pull/2001.

        Show
        James Iry added a comment - Changed my mind about the riskiness of the nulling version. It's now in https://github.com/scala/scala/pull/2001 .

          People

          • Assignee:
            James Iry
            Reporter:
            Ingo Maier
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development