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

InflatedMonitorsClosure repeats walks of StackFrameInfo and should cache instead

    XMLWordPrintable

Details

    • Enhancement
    • Resolution: Unresolved
    • P4
    • 23
    • 18
    • hotspot
    • generic
    • generic

    Description

      During the code review for:

      JDK-8273107 RunThese24H times out with "java.lang.management.ThreadInfo.getLockName()" is null

      [~rehn] noticed that InflatedMonitorsClosure can be inefficient.

      Here's the comments from that review:

      https://github.com/openjdk/jdk18/pull/25#discussion_r769378235

      and pasted here also:

      src/hotspot/share/services/threadService.cpp
            // Get the ObjectMonitors locked by this thread (if any):
            ObjectMonitorsHashtable::PtrList* list = table->get_entry(_thread);
            if (list != nullptr) {
              ObjectSynchronizer::monitors_iterate(&imc, list, _thread);
        
      Member
      @robehn robehn 3 days ago
      The InflatedMonitorsClosure walks the stack until object is found with this method:
      ThreadStackTrace::is_owned_monitor_on_stack(oop object)
      for every object...

      If you instead just collect all locks on stack with one pass you don't have to walk the stack over and over, which should be major speedup.
        
      Member
      Author
      @dcubed-ojdk dcubed-ojdk 2 days ago
      Sounds like an interesting RFE, but I'd prefer that be investigated
      separately from this bug.
        
      Member
      Author
      @dcubed-ojdk dcubed-ojdk 2 days ago
      Clarification: ThreadStackTrace::is_owned_monitor_on_stack() is not actually
      walking the (JavaThread's) stack, but it is walking the collection of
      StackFrameInfo objects that was gathered earlier in
      ThreadStackTrace::dump_stack_at_safepoint(). Adding a gather-locked-monitor
      helper object to the "add_stack_frame()" call would provide an easy hook and
      then InflatedMonitorsClosure could use the new helper object. Something like
      that anyway...
        
      Member
      @robehn robehn 2 days ago
      Yes, sorry, we loop the stack frame info for every object. I.e. a loop in a loop which is bad for time-complexity.
      If the we created a sorted list from the frame info, we can binary search it instead.
      Since a thread often only have at max a few locks, but could have hundreds of frames, just to be able skip looping over all frames without locks would be very beneficial.

      Attachments

        Issue Links

          Activity

            People

              dcubed Daniel Daugherty
              dcubed Daniel Daugherty
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated: