Uploaded image for project: '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)
        }
      }
      

        Attachments

          Activity

          Hide
          ureche 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
          ureche 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
          jamesiry 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
          jamesiry 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
          jamesiry James Iry added a comment - https://github.com/scala/scala/pull/1959
          Hide
          jamesiry 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
          jamesiry 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
          ureche 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
          ureche 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
          extempore Paul Phillips added a comment -

          That has to be the winner.

          Show
          extempore Paul Phillips added a comment - That has to be the winner.
          Hide
          jamesiry 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
          jamesiry 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
          jamesiry James Iry added a comment -
          Show
          jamesiry 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
          jamesiry 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
          jamesiry 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:
              jamesiry James Iry
              Reporter:
              imaier Ingo Maier
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: