Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8277824: Remove empty RefProcSubPhasesWorkerTimeTracker destructor #6555

Closed
wants to merge 2 commits into from
+5 −8
Closed

8277824: Remove empty RefProcSubPhasesWorkerTimeTracker destructor #6555

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38c66d6
rp-time-tracker
albertnetymk Nov 24, 2021
ae0bcf9
review
albertnetymk Nov 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 src/hotspot/share/gc/shared/referenceProcessorPhaseTimes.cpp
View file
Open in desktop
@@ -101,10 +101,7 @@ RefProcWorkerTimeTracker::~RefProcWorkerTimeTracker() {
RefProcSubPhasesWorkerTimeTracker::RefProcSubPhasesWorkerTimeTracker(ReferenceProcessor::RefProcSubPhases phase,
ReferenceProcessorPhaseTimes* phase_times,
uint worker_id) :
RefProcWorkerTimeTracker(phase_times->sub_phase_worker_time_sec(phase), worker_id) {
}

RefProcSubPhasesWorkerTimeTracker::~RefProcSubPhasesWorkerTimeTracker() {
_tracker(phase_times->sub_phase_worker_time_sec(phase), worker_id) {
}

RefProcPhaseTimeBaseTracker::RefProcPhaseTimeBaseTracker(const char* title,
8 src/hotspot/share/gc/shared/referenceProcessorPhaseTimes.hpp
View file
Open in desktop
@@ -103,23 +103,23 @@ class ReferenceProcessorPhaseTimes : public CHeapObj<mtGC> {
void print_all_references(uint base_indent = 0, bool print_total = true) const;
};

class RefProcWorkerTimeTracker : public CHeapObj<mtGC> {
class RefProcWorkerTimeTracker : public StackObj {
protected:
WorkerDataArray<double>* _worker_time;
double _start_time;
uint _worker_id;
public:
RefProcWorkerTimeTracker(WorkerDataArray<double>* worker_time, uint worker_id);
virtual ~RefProcWorkerTimeTracker();
~RefProcWorkerTimeTracker();
Copy link

@kimbarrett kimbarrett Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usual guideline is that the destructor for a baseclass should be either
public and virtual or protected and non-virtual. This prevents destructor
slicing by delete. That the base class in question is a StackObj mitigates
against that kind of error.

And indeed, if I recall correctly, because it's now a StackObj its destructor
can't be virtual, because that would run afoul of the induced "deleting
destructor".

There are some relevant warning options that I occasionally look at and
remember why I think they aren't right for us, so we can't ever enable them
(-Wdelete-non-virtual-dtor and -Wnon-virtual-dtor).

But all that is kind of irrelevant. The real design problem here is that
RefProcSubPhasesWorkerTimeTracker is derived from RefProcWorkerTimeTracker.
It should instead have a RefProceWorkerTimeTracker. That would eliminate
all the discussion about base class destructors, and would also make
RefProcWorkerTimeTracker no longer need to perform double-duty as both a base
class and as a concrete class (which is often problematic).

};

// Updates working time of each worker thread for a given sub phase.
class RefProcSubPhasesWorkerTimeTracker : public RefProcWorkerTimeTracker {
class RefProcSubPhasesWorkerTimeTracker : public StackObj {
RefProcWorkerTimeTracker _tracker;
public:
RefProcSubPhasesWorkerTimeTracker(ReferenceProcessor::RefProcSubPhases phase,
ReferenceProcessorPhaseTimes* phase_times,
uint worker_id);
~RefProcSubPhasesWorkerTimeTracker();
};

class RefProcPhaseTimeBaseTracker : public StackObj {