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

[Jon Masamitsu]

Stefan,

Thanks for looking at this.

I agree that the classes used in cmsCollector.cpp should all be in 
cmsCollector.cpp.  Should I move
that code or are there more basic issues?

Jon

On 8/14/2013 2:17 AM, Stefan Karlsson wrote:
> 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/
>