Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v4]
On Wed, 31 Jan 2024 23:07:16 GMT, Chris Plummer wrote: >> I noticed that "clhsdb jstack" seemed to hang when I attached to process >> with a somewhat large heap. It had taken over 10 minutes when I finally >> decided to have a look at the SA process (using bin/jstack), which came up >> with the following in the stack: >> >> >> at >> sun.jvm.hotspot.oops.ObjectHeap.iterateObjectsOfKlass([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36) >> at >> sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/StackTrace.java:71) >> >> >> This code is meant to print information about j.u.c. locks. It it works by >> searching the entire java heap for instances of AbstractOwnableSynchronizer. >> This is a very expensive operation because it means not only does SA need to >> read in the entire java heap, but it needs to create a Klass mirror for >> every heap object so it can determine its type. >> >> Our testing doesn't seem to run into this slowness problem because "clhsdb >> jstack" only iterates over the heap if AbstractOwnableSynchronizer has been >> loaded, which it won't be if no j.u.c. locks have been created. This seems >> to be the case wherever we use "clhsdb jstack" in testing. We do have some >> tests that likely result in j.u.c locks, but they all use "jhsdb jstack", >> which doesn't have this issue (it requires use of the --locks argument to >> enable printing of j.u.c locks). >> >> This issue was already called out in >> [JDK-8262098](https://bugs.openjdk.org/browse/JDK-8262098). For this CR I am >> proposing that "clhsdb jstack" not include j.u.c lock scanning by default, >> and add the -l argument to enable it. This will make it similar to >> bin/jstack, which also has a -l argument, and to "clhsdb jstack", which has >> a --locks argument (which maps internally to the Jstack.java -l argument). >> >> The same changes are also being made to "clhsdb pstack". >> >> Tested with all tier1, tier2, and tier5 svc tests. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > simplify regex passed to String.split(). Thanks for the reviews Kevin and Alex! - PR Comment: https://git.openjdk.org/jdk/pull/17479#issuecomment-1922065831
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v4]
On Wed, 31 Jan 2024 23:07:16 GMT, Chris Plummer wrote: >> I noticed that "clhsdb jstack" seemed to hang when I attached to process >> with a somewhat large heap. It had taken over 10 minutes when I finally >> decided to have a look at the SA process (using bin/jstack), which came up >> with the following in the stack: >> >> >> at >> sun.jvm.hotspot.oops.ObjectHeap.iterateObjectsOfKlass([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70) >> at >> sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36) >> at >> sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/StackTrace.java:71) >> >> >> This code is meant to print information about j.u.c. locks. It it works by >> searching the entire java heap for instances of AbstractOwnableSynchronizer. >> This is a very expensive operation because it means not only does SA need to >> read in the entire java heap, but it needs to create a Klass mirror for >> every heap object so it can determine its type. >> >> Our testing doesn't seem to run into this slowness problem because "clhsdb >> jstack" only iterates over the heap if AbstractOwnableSynchronizer has been >> loaded, which it won't be if no j.u.c. locks have been created. This seems >> to be the case wherever we use "clhsdb jstack" in testing. We do have some >> tests that likely result in j.u.c locks, but they all use "jhsdb jstack", >> which doesn't have this issue (it requires use of the --locks argument to >> enable printing of j.u.c locks). >> >> This issue was already called out in >> [JDK-8262098](https://bugs.openjdk.org/browse/JDK-8262098). For this CR I am >> proposing that "clhsdb jstack" not include j.u.c lock scanning by default, >> and add the -l argument to enable it. This will make it similar to >> bin/jstack, which also has a -l argument, and to "clhsdb jstack", which has >> a --locks argument (which maps internally to the Jstack.java -l argument). >> >> The same changes are also being made to "clhsdb pstack". >> >> Tested with all tier1, tier2, and tier5 svc tests. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > simplify regex passed to String.split(). Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17479#pullrequestreview-1855155769
Re: RFR: 8324066: "clhsdb jstack" should not by default scan for j.u.c locks because it can be very slow [v4]
> I noticed that "clhsdb jstack" seemed to hang when I attached to process with > a somewhat large heap. It had taken over 10 minutes when I finally decided to > have a look at the SA process (using bin/jstack), which came up with the > following in the stack: > > > at > sun.jvm.hotspot.oops.ObjectHeap.iterateObjectsOfKlass([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ObjectHeap.java:117) > at > sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.fillLocks([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:70) > at > sun.jvm.hotspot.runtime.ConcurrentLocksPrinter.([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/ConcurrentLocksPrinter.java:36) > at > sun.jvm.hotspot.tools.StackTrace.run([jdk.hotspot.agent@23-internal](jdk.hotspot.agent@23-internal)/StackTrace.java:71) > > > This code is meant to print information about j.u.c. locks. It it works by > searching the entire java heap for instances of AbstractOwnableSynchronizer. > This is a very expensive operation because it means not only does SA need to > read in the entire java heap, but it needs to create a Klass mirror for every > heap object so it can determine its type. > > Our testing doesn't seem to run into this slowness problem because "clhsdb > jstack" only iterates over the heap if AbstractOwnableSynchronizer has been > loaded, which it won't be if no j.u.c. locks have been created. This seems to > be the case wherever we use "clhsdb jstack" in testing. We do have some tests > that likely result in j.u.c locks, but they all use "jhsdb jstack", which > doesn't have this issue (it requires use of the --locks argument to enable > printing of j.u.c locks). > > This issue was already called out in > [JDK-8262098](https://bugs.openjdk.org/browse/JDK-8262098). For this CR I am > proposing that "clhsdb jstack" not include j.u.c lock scanning by default, > and add the -l argument to enable it. This will make it similar to > bin/jstack, which also has a -l argument, and to "clhsdb jstack", which has a > --locks argument (which maps internally to the Jstack.java -l argument). > > The same changes are also being made to "clhsdb pstack". > > Tested with all tier1, tier2, and tier5 svc tests. Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: simplify regex passed to String.split(). - Changes: - all: https://git.openjdk.org/jdk/pull/17479/files - new: https://git.openjdk.org/jdk/pull/17479/files/6c24a9a2..929de70f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17479&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17479&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17479.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17479/head:pull/17479 PR: https://git.openjdk.org/jdk/pull/17479
