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