United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-6511756 forte_is_valid_method() should call CollectedHeap::is_in_reserved() rather than is_in()
JDK-6511756 : forte_is_valid_method() should call CollectedHeap::is_in_reserved() rather than is_in()

Details
Type:
Bug
Submit Date:
2007-01-10
Status:
Closed
Updated Date:
2011-03-07
Project Name:
JDK
Resolved Date:
2011-03-07
Component:
vm-legacy
OS:
generic,solaris_10
Sub-Component:
jvmpi
CPU:
sparc,generic
Priority:
P3
Resolution:
Fixed
Affected Versions:
6,6u4
Fixed Versions:
hs11 (b07)

Related Reports
Backport:
Backport:
Backport:
Duplicate:
Relates:
Relates:
Relates:

Sub Tasks

Description
See comments section.

                                    

Comments
SUGGESTED FIX

Simple one-line change:

bool forte_is_valid_method(methodOop method) {

  if (method == NULL ||
      // The methodOop is extracted via an offset from the current
      // interpreter frame. With AsyncGetCallTrace() the interpreter
      // frame may still be under construction so we need to make
      // sure that we got an aligned oop before we try to use it.
      !Space::is_aligned(method) ||
      !Universe::heap()->is_in((void*)method) ||
                         ^^^^^^
                         change to call is_in_reserved(..) instead.

Note that calling is_in() is too expensive and would make the
profiling code too heacyweight and distort the application execution
profile.
                                     
2007-01-10
SUGGESTED FIX

Note that the above change is safe, because there's a further check
further down which protects our use of the putative methodOop,
in that we check method->is_perm_and_alloced() which does the
stronger test. The stronger (and potentially more expensive) test
further down is fine presumably because it will be hit less
often and also because in all current and near-future
HotSpot configurations (including G1) the committed
perm gen is a convex interval of the heap (hmm, i don't
know about repercussions for shared perm gens, but may be we
don't care much about that at them moment; in any case, i'll
check on that).
                                     
2007-07-24
EVALUATION

Note: the assert affects only the GenCollectedHeap side of things.
Current GenCollectedHeap collectors do not actually have a performance
problem (despite the pessimistic assert). The assert really belongs
in CollectedHeap or specific sub-classes, for example G1CollectedHeap(?),
that have non-convex committed regions for which is_in() may be more
expensive to compute in general.
                                     
2007-07-24
SUGGESTED FIX

preliminary webrev:-

http://analemma.sfbay.sun.com/net/spot/workspaces/ysr/merge/webrev/
                                     
2007-09-06
EVALUATION

The assert referred to above has still been left in place,
but the is_valid_method() has been moved into a CollectedHeap
interface. The need for is_in() or is_in_reserved() from this
method is gone, and we just rely on is_permanent().
                                     
2007-09-13
SUGGESTED FIX

Event:            putback-to
Parent workspace: /net/jano2.sfbay/export2/hotspot/ws/main/gc_baseline
                  (jano2.sfbay:/export2/hotspot/ws/main/gc_baseline)
Child workspace:  /net/prt-web.sfbay/prt-workspaces/20070912141354.ysr.merge/workspace
                  (prt-web:/net/prt-web.sfbay/prt-workspaces/20070912141354.ysr.merge/workspace)
User:             ysr

Comment:

---------------------------------------------------------

Job ID:                 20070912141354.ysr.merge
Original workspace:     neeraja:/net/spot/workspaces/ysr/merge
Submitter:              ysr
Archived data:          /net/prt-archiver.sfbay/data/archived_workspaces/main/gc_baseline/2007/20070912141354.ysr.merge/
Webrev:                 http://prt-web.sfbay.sun.com/net/prt-archiver.sfbay/data/archived_workspaces/main/gc_baseline/2007/20070912141354.ysr.merge/workspace/webrevs/webrev-2007.09.12/index.html

Fixed 6511756: forte_is_valid_method() should call CollectedHeap::is_in_reserved() rather than is_in()
Fixed 6602306: PS, ParNew: reference processor's _is_alive_non_header field should be left NULL

  webrev: http://analemma.sfbay/net/spot/workspaces/ysr/merge/webrev
 
The first allows the so-called interface collectors to be used
with Analyzer/Collector. We took the opportunity to consolidate
and standardize the check for a "valid" method oop under a CollectedHeap
interface. Some simplification followed because tests
with "collect -j" using refworkload indicated that the
weaker is_in{_reserved}() filter over the entire heap was
not adding any additional value. (In that sense the
synopsis is now somewhat obsolete, and might have been
better rephrased "Sun Studio Collector asserts in GenCollectedHeap::is_in().")

The second fixes an inadvertent change to reference
processor construction (in the course of the recent extension
of parallel reference processing to all parallel collectors)
for the case of ParNew and PS, which led to a redundant test
during reference discovery. The measured difference in performance is,
however, in the noise.

Reviewed by: Dan Daugherty, Steve Goldman, Jon Masamitsu

Fix verified: yes

Verification testing:
  collect -j java -XX:+UseSerialGC (for instance)

Other testing:
  collect -j on: with refworkload benchmarks running all garbage collectors
  PRT
  Dan's Forte Stress Kit
(Failures were seen on x86 related to 6380402/6603919, the
existence of which predates and is orthogonal to the changes
in the present CR; the Forte Stress Kit detected a few
hangs when running with Java2Demo, which also similarly
predate and appear orthogonal to this change.)

Files:
update: src/share/vm/gc_implementation/parNew/parNewGeneration.cpp
update: src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp
update: src/share/vm/gc_interface/collectedHeap.hpp
update: src/share/vm/gc_interface/collectedHeap.inline.hpp
update: src/share/vm/memory/compactingPermGenGen.hpp
update: src/share/vm/oops/oop.hpp
update: src/share/vm/oops/oop.inline2.hpp
update: src/share/vm/prims/forte.cpp
update: src/share/vm/runtime/fprofiler.cpp

Examined files: 4034

Contents Summary:
       9   update
    4025   no action (unchanged)
                                     
2007-09-13



Hardware and Software, Engineered to Work Together