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

Stefan Karlsson stefan.karlsson at oracle.com
Thu Aug 15 07:33:37 UTC 2013


On 8/15/13 5:01 AM, Jon Masamitsu wrote:
> 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?
I think they should be moved, but I fear that you'll end up with a 
cmsCollector.cpp that is almost as big as the current 
concurrentMarkSweepGeneration.cpp ...

thanks,
StefanK
>
> 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/
>>
>




More information about the hotspot-gc-dev mailing list