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