Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8175116

jtreg agentvms uses more virtual address space in langtool/test :tier1 runs

    Details

    • Type: Bug
    • Status: In Progress
    • Priority: P2
    • Resolution: Unresolved
    • Affects Version/s: 9, 10
    • Fix Version/s: 9
    • Component/s: tools

      Description

      The changes made under JDK-8166002 emulated the old client build in the win 32 environment with reduced address space. Before that change was made, several very stable langtools regression tests had high frequency intermittent failures, requiring updates to the tests to pass reliably again (JDK-8168499, JDK-8163999, JDK-8165193).

      After JDK-8166002 was integrated, the test updates were reverted (JDK-8173305) and the failures were seen again, requiring the test updates to be reapplied (JDK-8174059).

      Therefore, at least in some cases, the previous virtual address space behavior of the client compiler is not fully replicated.

      From analysis, it was concluded that this is not emulated client issue.
      This behavior is observed from 9b113, JDK-8152733 integration.

        Issue Links

          Activity

          Hide
          redestad Claes Redestad added a comment -
          With no weak references to it, then once an object becomes garbage it can be GCd more or less immediately; a WeakReference'd object however, once there's no longer any strong reference to it, will first have to be processed by the GC and put on a reference queue, which then needs to be processed (in java; for this specific instance see java.lang.reflect.WeakPairMap) before it can ultimately be GCd on the next cycle or STW pause - this means there's an added latency in how promptly each object is finalized, sometimes many times longer than for a corresponding strongly referenced object. The evidence suggests this was what happened here, and that it's enough to blow up the amount of native memory retained at any particular time in a test like TreePosTest by a significant factor.
          Show
          redestad Claes Redestad added a comment - With no weak references to it, then once an object becomes garbage it can be GCd more or less immediately; a WeakReference'd object however, once there's no longer any strong reference to it, will first have to be processed by the GC and put on a reference queue, which then needs to be processed (in java; for this specific instance see java.lang.reflect.WeakPairMap) before it can ultimately be GCd on the next cycle or STW pause - this means there's an added latency in how promptly each object is finalized, sometimes many times longer than for a corresponding strongly referenced object. The evidence suggests this was what happened here, and that it's enough to blow up the amount of native memory retained at any particular time in a test like TreePosTest by a significant factor.
          Hide
          jlahoda Jan Lahoda added a comment -
          I was looking at CheckAttributedTree, and the reason why it works is that it will actually initialize the annotation processing machinery only once, i.e. create the URLClassLoader only once, avoiding this problem. This is not quite right, but OK for most tests. I note that this is similar to adding -proc:none to this test, which was done.

          What I think should be done:
          a) apply Claes' patch (as javac indeed seems to be doing something it does not need to do). But, this does not solve the real problem of not releasing the resources in time, so in principle some other change soon can break this again.
          b) add (or at least very serious consider adding) JavacTask.close, to be called when the task is not needed anymore, and use it where appropriate.
          c) convert tests to ComboTestHelper (or use "-proc:none") where appropriate (this is mostly orthogonal to the above, and do some degree independent of this bug).

          The main question is when to do these - I don't think we can realistically do b) for 9, but I think we should try with a).

          One question, though: do the tests fail now? If not, then nothing really wrong will happen if a) is in some 9 update, right?

          Comments?
          Show
          jlahoda Jan Lahoda added a comment - I was looking at CheckAttributedTree, and the reason why it works is that it will actually initialize the annotation processing machinery only once, i.e. create the URLClassLoader only once, avoiding this problem. This is not quite right, but OK for most tests. I note that this is similar to adding -proc:none to this test, which was done. What I think should be done: a) apply Claes' patch (as javac indeed seems to be doing something it does not need to do). But, this does not solve the real problem of not releasing the resources in time, so in principle some other change soon can break this again. b) add (or at least very serious consider adding) JavacTask.close, to be called when the task is not needed anymore, and use it where appropriate. c) convert tests to ComboTestHelper (or use "-proc:none") where appropriate (this is mostly orthogonal to the above, and do some degree independent of this bug). The main question is when to do these - I don't think we can realistically do b) for 9, but I think we should try with a). One question, though: do the tests fail now? If not, then nothing really wrong will happen if a) is in some 9 update, right? Comments?
          Hide
          darcy Joe Darcy added a comment -
          To see if the changes suggested fix the precise problem that was observed, annotation should be disabled for the set of tests (as has already been done for TreePosTest) and then the /othervm workaround in the test tags should be removed.
          Show
          darcy Joe Darcy added a comment - To see if the changes suggested fix the precise problem that was observed, annotation should be disabled for the set of tests (as has already been done for TreePosTest) and then the /othervm workaround in the test tags should be removed.
          Hide
          jjg Jonathan Gibbons added a comment -
          [~jlahoda]

          I agree with your general approach to look at the Big Picture, before doing too many point fixes that workaround the problem.

          I would add an item (d) to your list.
          d) investigate whether it is possible to proactively close the URLClassLoader early (i.e. as soon as possible) if it determined that there are no processors to be found and run.

          In other words, from an AP point of view there are two distinct issues in this general thread:
          1. opening processor class loaders "unnecessarily" when we know they will not be required ... this is a simple performance issue.
          2. not closing processor class loaders in a timely fashion once they have been opened ... this leads to a GC problem.
          Show
          jjg Jonathan Gibbons added a comment - [~jlahoda] I agree with your general approach to look at the Big Picture, before doing too many point fixes that workaround the problem. I would add an item (d) to your list. d) investigate whether it is possible to proactively close the URLClassLoader early (i.e. as soon as possible) if it determined that there are no processors to be found and run. In other words, from an AP point of view there are two distinct issues in this general thread: 1. opening processor class loaders "unnecessarily" when we know they will not be required ... this is a simple performance issue. 2. not closing processor class loaders in a timely fashion once they have been opened ... this leads to a GC problem.
          Hide
          jjg Jonathan Gibbons added a comment -
          [~redestad]

          Thanks for your explanation. I agree that the call of ensureReadable should be removed, but I still see it as something of a "canary in the coal mine" indicator that a seemingly innocuous call like ensureReadable can cause such big undocumented side effects. Now, we can separately discuss whether that is a problem in the immediate implementation of ensureReadable or a problem deeper down in the responsiveness of GC, but I still think there's an issue there.
          Show
          jjg Jonathan Gibbons added a comment - [~redestad] Thanks for your explanation. I agree that the call of ensureReadable should be removed, but I still see it as something of a "canary in the coal mine" indicator that a seemingly innocuous call like ensureReadable can cause such big undocumented side effects. Now, we can separately discuss whether that is a problem in the immediate implementation of ensureReadable or a problem deeper down in the responsiveness of GC, but I still think there's an issue there.

            People

            • Assignee:
              jlahoda Jan Lahoda
              Reporter:
              darcy Joe Darcy
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Due:
                Created:
                Updated: