Request for review (m) - 8022725:Refactor code in concurrentMarkSweepGeneration.cpp

Stefan Karlsson stefan.karlsson at oracle.com
Wed Aug 14 09:17:07 UTC 2013


Hi Jon,

I like that we are splitting up this file, however I don't like the way 
it has been done.

We now end up having a lot of classes, mostly closures, declared and 
defined in concurrentMarkSweepGeneration.[h|c|inline.h]pp that are only 
used in cmsCollector.cpp.

For example, take a look at MarkFromRootsClosure:
$ grep -r "MarkFromRootsClosure" src/ | grep -v "Par_MarkFromRootsClosure"
src/share/vm/gc_implementation/concurrentMarkSweep/cmsCollector.cpp: 
MarkFromRootsClosure markFromRootsClosure(this, _span,
src/share/vm/gc_implementation/concurrentMarkSweep/cmsCollector.cpp: 
MarkFromRootsClosure markFromRootsClosure(this, _span, &_markBitMap,
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp:class 
MarkFromRootsClosure: public BitMapClosure {
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp: 
MarkFromRootsClosure(CMSCollector* collector, MemRegion span,
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp:MarkFromRootsClosure::MarkFromRootsClosure(CMSCollector* 
collector,
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp:void 
MarkFromRootsClosure::reset(HeapWord* addr) {
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp:bool 
MarkFromRootsClosure::do_bit(size_t offset) {
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp:void 
MarkFromRootsClosure::do_yield_work() {
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp:void 
MarkFromRootsClosure::scanOopsInOop(HeapWord* ptr) {
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp: 
HeapWord* finger, MarkFromRootsClosure* parent) :
src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp:class 
MarkFromRootsClosure;
src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp:// 
the closure MarkFromRootsClosure.
src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp: 
MarkFromRootsClosure* const
src/share/vm/gc_implementation/concurrentMarkSweep/cmsOopClosures.hpp: 
MarkFromRootsClosure* parent);
src/share/vm/gc_implementation/concurrentMarkSweep/cmsCollector.hpp: 
friend class MarkFromRootsClosure;          //  -- ditto --
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.inline.hpp:inline 
void MarkFromRootsClosure::do_yield_check() {

I'm not going to look at the entire patch, since I think we need to 
figure out how to proceed with this issue.


Some smaller things:

The CMSBitMap implementation should probably be moved to a 
cmsBitMap.cpp, or cmsBitMap.inline.hpp, file.

The includes you've added are not sorted. For example:
   #include "precompiled.hpp"
+ #include "gc_implementation/concurrentMarkSweep/cmsBitMap.inline.hpp"
   #include "gc_implementation/concurrentMarkSweep/cmsLockVerifier.hpp"
   #include 
"gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.hpp"
+ #include "gc_implementation/concurrentMarkSweep/cmsCollector.inline.hpp"

thanks,
StefanK

On 08/09/2013 07:39 PM, Jon Masamitsu wrote:
> 8022725:Refactor code in concurrentMarkSweepGeneration.cpp
>
> concurrentMarkSweepGeneration.cpp is a rather large file and should be 
> refactored (split into multiple
> files) for easier maintenance.
>
> There are no intended semantic changes. Only reorganization and moving 
> of code.
>
> I've been investigating some issues with compiler optimizations on 
> this file and the refactoring will make that doable.
>
> http://cr.openjdk.java.net/~jmasa/8022725/webrev.00/




More information about the hotspot-gc-dev mailing list