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

8231672: Simplify the reference processing parallelization framework #2782

Closed
wants to merge 11 commits into from

Conversation

@lkorinth
Copy link
Contributor

@lkorinth lkorinth commented Mar 1, 2021

With the change of parallel gc to use the same workgang implementation that other gcs uses, it is possible to remove the abstraction used to hide different worker thread management implementations (TaskExecutor).

At the same time as removing the TaskExecutor, also remove part of the special casing (mostly code duplication), for single threaded execution.

I consider the new method prepare_run_task that modifies the state of the context a step backwards from what was. However, I think removing the Executor with its proxy-tasks and removing separate code paths for serial execution makes the change worth this step back. We also removes ~270 lines of code.

Some comments:

  1. I use references in some places where they can replace pointers. I could go much further here, but I did not want to bloat the pull request, maybe later change all closures to be references as well? Should I create enhancements for this?

  2. I added an enum class ThreadModel instead of using a boolean, this could also be used in many more places. I dislike sending lots of bools with a comment like true /* _is_mt */. It also adds type safety if a method takes many bools. However, with this limited change, and not many hard-coded bools, it feels a bit overkill and I am willing to remove the enum, what do you think?

Testing:
hotspot_gc and tier 1-3 has passed earlier versions before minor cleanups. I intend to re-run tests after review changes.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8231672: Simplify the reference processing parallelization framework

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2782/head:pull/2782
$ git checkout pull/2782

Update a local copy of the PR:
$ git checkout pull/2782
$ git pull https://git.openjdk.java.net/jdk pull/2782/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2782

View PR using the GUI difftool:
$ git pr show -t 2782

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2782.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 1, 2021

👋 Welcome back lkorinth! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Mar 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 1, 2021

@lkorinth The following label will be automatically applied to this pull request:

  • hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the hotspot-gc label Mar 1, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 1, 2021

Loading

Copy link
Contributor

@tschatzl tschatzl left a comment

I use references in some places where they can replace pointers. I could go much further here, but I did not
want to bloat the pull request, maybe later change all closures to be references as well? Should I create
enhancements for this?

If necessary, generally do such changes (vs. the reference processing refactoring) in extra CRs. Changing pointers to references seems completely unrelated.

Fwiw, my personal opinion is to not do that at all for code that has lots of connections with existing code as this results in an awkward mixture in these areas between old an new code which only makes things hard to read (and adds another question when interfacing that code, do I need a reference or a pointer now?), but others may have other opinions.
Also it generally hides the information at the call site whether something is actually passed as reference or as value (at least for classes/structs). Of course, as usual there is no hard rule, and I am good if others think it's the way to go.

Loading

G1CMRefProcClosureContext(
uint max_workers,
G1CollectedHeap& g1h,
G1ConcurrentMark& cm)
: _max_workers(max_workers),
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Please adopt a constructor formatting style already present in the file at least. Please do not add new ones.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 4, 2021

Choose a reason for hiding this comment

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

I will move uint max_workers to the first line and align the constructor arguments to the first parameter, but leave the rest as is. I will try to use that style later as well.

Loading

if (_tm == ThreadModel::Single) {
return ::new (&_serial_keep_alive) G1CopyingKeepAliveClosure(&_g1h, _pss.state_for_worker(0));
}
return ::new (&_keep_alive[worker_id]) G1CopyingKeepAliveClosure(&_g1h, _pss.state_for_worker(worker_id));
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

I kind of prefer the symmetric
if (something) { return a; } else { return b; }

style than this "dangling" return as the returns are on the same indentation level. Feel free to ignore in multiple places.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 4, 2021

Choose a reason for hiding this comment

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

I will fix that.

Loading

G1CMRefProcTaskExecutor par_task_executor(_g1h, this,
_g1h->workers(), active_workers);
AbstractRefProcTaskExecutor* executor = (processing_is_mt ? &par_task_executor : NULL);
// reference processing context.
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Initial capitalization.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 4, 2021

Choose a reason for hiding this comment

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

I just remove the comment, it only add noise.

Loading

@@ -230,6 +229,39 @@ void G1FullCollector::update_attribute_table(HeapRegion* hr) {
}
}

class G1FullRefProcClosureContext : public AbstractClosureContext {
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Please add "GC" after "Full" because this naming begs the question what a "partial" context would look like.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 4, 2021

Choose a reason for hiding this comment

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

I will do that.

Loading

assert(worker_id < _max_workers, "sanity");
return ::new (&_keep_alive[index(worker_id, _tm)]) G1CMKeepAliveAndDrainClosure(&_cm, _cm.task(index(worker_id, _tm)), _tm == ThreadModel::Single );
}
VoidClosure* complete_gc(uint worker_id) {
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Inconsistent spacing between methods, sometimes there is a newline, sometimes not. I would prefer the additional newline. This is also the case in the other file.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 4, 2021

Choose a reason for hiding this comment

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

I will do that.

Loading

ParCompactionManager* cm = ParCompactionManager::gc_thread_compaction_manager(worker_id);
return ::new (&_keep_alive[worker_id]) PCMarkAndPushClosure(cm);
}
VoidClosure* complete_gc(uint worker_id) {
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Newline between method definitions.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 4, 2021

Choose a reason for hiding this comment

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

I will do that.

Loading

~G1FullRefProcClosureContext() {
FREE_C_HEAP_ARRAY(G1FullKeepAliveClosure, _keep_alive);
}
BoolObjectClosure* is_alive(uint worker_id) {
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Newlines between definitions.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 4, 2021

Choose a reason for hiding this comment

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

I will do that.

Loading

WorkGang* _workers;
class G1STWRefProcClosureContext : public AbstractClosureContext {
uint _max_workers;
uint _queues;
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Please avoid lining up member names. That is hard to read. Group them by functionality/contents.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 5, 2021

Choose a reason for hiding this comment

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

Agree, will remove all spaces.

Loading

{
g1h->ref_processor_stw()->set_active_mt_degree(workers->active_workers());
G1STWRefProcClosureContext(
uint max_workers,
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Parameter indentation.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 5, 2021

Choose a reason for hiding this comment

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

will fix.

Loading

}

BoolObjectClosure* is_alive(uint worker_id) { return &_is_alive; }
OopClosure* keep_alive(uint worker_id) {
Copy link
Contributor

@tschatzl tschatzl Mar 4, 2021

Choose a reason for hiding this comment

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

Newline between definitions.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 5, 2021

Choose a reason for hiding this comment

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

Will fix.

Loading

@lkorinth
Copy link
Contributor Author

@lkorinth lkorinth commented Mar 5, 2021

I have tried to fix the formatting mess (sorry) and renaming of classes. I hope this looks better.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 6, 2021

@lkorinth this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout _8231672
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Loading

@openjdk openjdk bot removed the merge-conflict label Mar 8, 2021
Copy link
Contributor

@tschatzl tschatzl left a comment

I think the code looks better now. Thanks.

Loading

@@ -180,55 +180,64 @@ class PSKeepAliveClosure: public OopClosure {
class PSEvacuateFollowersClosure: public VoidClosure {
private:
PSPromotionManager* _promotion_manager;
TaskTerminator* _maybe_terminator;
Copy link
Contributor

@tschatzl tschatzl Mar 9, 2021

Choose a reason for hiding this comment

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

Not really seeing the advantage: if you want to make sure that something isn't nullptr when it's used, put an assert before the use, then it should be clear (and runtime-checked) that it's not valid; even if it were nullptr, testing should have found (i.e. exercised that path) that anyway and failed fast (by dereferencing that null value).

A reviewer needs to look if values may actually be nullptr in that code path anyway regardless of the name.

Maybe someone else likes this better though, so no opinion here.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 9, 2021

@lkorinth This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8231672: Simplify the reference processing parallelization framework

Reviewed-by: tschatzl, ayang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Mar 9, 2021
Copy link

@kimbarrett kimbarrett left a comment

The "Context" suffix isn't suggestive of what it does for me. "context" suggests scope or surroundings. I think "Group" would be better; as this is a collection of inter-related things. I thought of some others, but many are already overused while others were way to obscure. Another possibility is "xxxClosureContext" => "xxxClosures", though perhaps that pluralization is too subtle.

I think the life-cycle management of the closures by the context objects is a problem. I'm guessing this is to lazily construct the closures? But as used this may be constructing closures over storage containing an already constructed closure, without calling the destructor. This is all UB if the destructors have any relevant side effects (3.8/4). Is that guaranteed for all such closures? I'm not sure I like that requirement on the closure types, and certainly not without some documentation making that explicit. We can't use is_trivial_destructor<> as a safety net since these closures have non-trivial destructors (they are virtual).

Lazy construction and as-needed destruction could be done via having a parallel set of is_constructed flags. Context destructor cleans up accordingly.

Or maybe have a high-water construction tracker and have prepare_run_task do construction to advance that high-water mark as needed. Again, Context destructor cleans up accordingly.

In the various context classes, I think the virtual overrides should be explicitly marked "virtual", as that's usually made explicit in our code. (And I've finally sent out JDK-8254050.)

I think there's confusion over determining the number of workers to use. There are various places that are using active_workers() where total_workers() might be better, for the same reasons as in
https://bugs.openjdk.java.net/browse/JDK-8258985
But I think that same confusion exists in the old code, so I'm okay with treating this as a pre-existing issue for a different change.

Loading

virtual OopClosure* keep_alive(uint worker_id) = 0;
virtual VoidClosure* complete_gc(uint worker_id) = 0;
virtual void prepare_run_task(uint queue_count, ThreadModel tm, bool marks_oops_alive) = 0;
uint index(uint id, ThreadModel tm) { return (tm==ThreadModel::Single)?0:id; }

Choose a reason for hiding this comment

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

I'd like more spaces around operators and punctuation in that value expression.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 23, 2021

Choose a reason for hiding this comment

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

The code was updated because Thomas also found it, are you okay with the current code?

Loading

Choose a reason for hiding this comment

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

Yes, updated code is fine.

Loading


class AbstractClosureContext {

Choose a reason for hiding this comment

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

This seems like an overly general name. This is related to reference
processing I think (not going to be used elsewhere), so I think should be
named accordingly. ClosureContext also seems odd. Maybe something
like AbstractRefProcOperations? Handlers (slight bleh)?

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 23, 2021

Choose a reason for hiding this comment

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

This is not the current name. This was updated to AbstractRefProcClosureContext earlier. I will update to AbstractRefProcOperations if you prefer that. I think your suggestion is better.

Loading

Choose a reason for hiding this comment

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

I like the Operations suffix better too.

Loading

OopClosure& keep_alive,
VoidClosure& complete_gc) {
virtual void work(uint worker_id) {
ResourceMark rm;

Choose a reason for hiding this comment

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

There are a number of places like here where a ResourceMark has been introduced. I've not been able to figure out what these are for. And that makes me wonder whether they are in the right place.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 23, 2021

Choose a reason for hiding this comment

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

The ResourceMarks was used in some of the previous nested closures. See removal of G1STWRefProcTaskProxy and G1CMRefProcTaskProxy.

Loading

Choose a reason for hiding this comment

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

Yes, and I'm not sure why any of them exist, other than some paranoia that there might be some not properly scoped resource allocations somewhere inside that we want to squash on the way out of the thread. That is, these seem like just hiding potential bugs. But you probably don't want to chase down any actual bugs of that sort as part of this change, so okay, leave them.

Loading

@@ -2023,7 +2023,7 @@ static void mark_from_roots_work(ParallelRootType::Value root_type, uint worker_
cm->follow_marking_stacks();
}

static void steal_marking_work(TaskTerminator& terminator, uint worker_id) {
void steal_marking_work(TaskTerminator& terminator, uint worker_id) {

Choose a reason for hiding this comment

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

If this needs to be made non-file-scoped then it should be moved into some relevant class.

Loading

Choose a reason for hiding this comment

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

Sorry, never mind, the minimal diff confused me about the context. We're in class scope here, not at namespace scope.

Loading

Choose a reason for hiding this comment

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

Never mind, I was confused by the minimal diff. This is already at class scope, not at file scope.

Loading


virtual void do_void() {
assert(_promotion_manager != NULL, "Sanity");
assert(_promotion_manager != nullptr, "Sanity");

Choose a reason for hiding this comment

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

I'd prefer this assert was in the constructor.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 23, 2021

Choose a reason for hiding this comment

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

I agree. I further prefer not having an assert and instead using a reference. I also fear doing unrelated changes will be rejected. This is no new code of mine, it is added so that nullptr is used consistently after push-back from Thomas.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 23, 2021

Choose a reason for hiding this comment

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

What would you prefer?

  1. keep as is now.
  2. move to constructor.
  3. use a reference instead.

Loading

Choose a reason for hiding this comment

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

I would probably move it to the constructor, but I don't feel strongly about it.

Loading

VoidClosure& complete_gc) = 0;

bool marks_oops_alive() const { return _marks_oops_alive; }
virtual BoolObjectClosure* is_alive(uint worker_id) = 0;

Choose a reason for hiding this comment

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

Missing destructor for base class - should be public and virtual or non-public and non-virtual. As these seem to always be stack allocated, a protected default destructor would be appropriate.

Loading


class AbstractRefProcClosureContext {

Choose a reason for hiding this comment

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

I think these are always stack allocated, so derive from StackObj? (To at least signal intent.)

Loading

Copy link
Contributor Author

@lkorinth lkorinth Mar 23, 2021

Choose a reason for hiding this comment

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

Yes, will change.

Loading

@lkorinth
Copy link
Contributor Author

@lkorinth lkorinth commented Apr 16, 2021

Hi, and sorry for the long silence from me.

I got feedback from Albert on improving the structure of this change and to use proxy classes instead of ClosureContexts. This will solve the problem with heap allocated closures and also improve readability IMO.

The change is big, so I will not push it to this review thread before you agree with that it is the right way to go, but you can see the change here: lkorinth@5cdddac

If you agree with me, I will push that change to this review.

Loading

@lkorinth
Copy link
Contributor Author

@lkorinth lkorinth commented Apr 21, 2021

So in addition to using proxy classes instead of ClosureContexts, I have also fixed an earlier mistake of mine when getting the ParCompactionManager and PSPromotionManager in the VM thread I will no longer use the index of worker_id and instead use the dedicated manager for the VM thread.

Loading

marks_oops_alive ? "true" : "false");

proxy_task.prepare_run_task(task, num_queues(), processing_is_mt() ? RefProcThreadModel::Multi : RefProcThreadModel::Single, marks_oops_alive);
if (gang != NULL && processing_is_mt()) {
Copy link
Member

@albertnetymk albertnetymk Apr 24, 2021

Choose a reason for hiding this comment

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

Since gang is only used in thin branch, moving it inside if might be better. Besides, the assertion on L779 could be replaced by the code below, IMO.

if (processing_is_mt()) {
  WorkGang* gang = Universe::heap()->safepoint_workers();
  assert(gang != nullptr, "...");
  ...
}

Loading

Copy link
Contributor Author

@lkorinth lkorinth Apr 27, 2021

Choose a reason for hiding this comment

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

Yes!

Loading

@@ -38,14 +39,15 @@ class ParallelCompactData;
class ParMarkBitMap;

class ParCompactionManager : public CHeapObj<mtGC> {
friend class CompactionWithStealingTask;
Copy link
Member

@albertnetymk albertnetymk Apr 24, 2021

Choose a reason for hiding this comment

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

CompactionWithStealingTask and UpdateAndFillClosure are dead code.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Apr 27, 2021

Choose a reason for hiding this comment

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

I will remove them.

Loading


// Abstract reference processing task to execute.
class AbstractRefProcTaskExecutor::ProcessTask {
class RefProcTask : StackObj {
Copy link
Member

@albertnetymk albertnetymk Apr 24, 2021

Choose a reason for hiding this comment

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

Maybe put some comments here, explaining what RefProcTask and RefProcProxyTask are, their interaction/relation.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Apr 27, 2021

Choose a reason for hiding this comment

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

I will try to do that.

Loading

uint index(uint id) const {
return (_tm == RefProcThreadModel::Single) ? 0 : id;
}
Copy link
Member

@albertnetymk albertnetymk Apr 24, 2021

Choose a reason for hiding this comment

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

This method is only used by G1; I wonder if it's possible to not include it in the API of RefProcProxyTask.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Apr 27, 2021

Choose a reason for hiding this comment

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

I could --- and maybe should --- but then I need to supply _tm as an argument, further I need to find a logical place to put the function and the declaration, and if I add it to referenceProcessor.?pp I have not made it less generic. If I add it to a G1 file, it seems to me arbitrary which to use. If you find a nice place to put it, I will move it, otherwise I will keep it where it is. I will not create a new file for the function and would in that case prefer to inline expand the code.

Loading

Copy link
Member

@albertnetymk albertnetymk Apr 27, 2021

Choose a reason for hiding this comment

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

Since the body of uint index(uint id) const is just one line, I was thinking the caller can just inline it. Taking G1FullGCRefProcProxyTask as an example.

before

G1FullKeepAliveClosure keep_alive(_collector.marker(index(worker_id)));
G1FollowStackClosure* complete_gc = _collector.marker(index(worker_id))->stack_closure();

after

auto index = _tm == RefProcThreadModel::Single ? 0 : worker_id;
auto marker = _collector.marker(index);
G1FullKeepAliveClosure keep_alive(marker);
G1FollowStackClosure* complete_gc = marker->stack_closure();

My concern with including uint index(uint id) const in the API of RefProcProxyTask is that its name/implementation is too generic. In the context of RefProcProxyTask, it's not obvious what id is, what index should return, and why (_tm == RefProcThreadModel::Single) ? 0 : id; is correct.

Loading

Copy link
Contributor Author

@lkorinth lkorinth Apr 27, 2021

Choose a reason for hiding this comment

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

I inlined it.

Loading

RefProcTotalPhaseTimesTracker tt(RefPhase1, phase_times, this);
process_soft_ref_reconsider(is_alive, keep_alive, complete_gc,
task_executor, phase_times);
RefProcTotalPhaseTimesTracker tt(RefPhase1, &phase_times, this);
Copy link
Member

@albertnetymk albertnetymk Apr 24, 2021

Choose a reason for hiding this comment

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

RefProcTotalPhaseTimesTracker doesn't really use this, right?

Loading

Copy link
Contributor Author

@lkorinth lkorinth Apr 27, 2021

Choose a reason for hiding this comment

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

Nice catch! Pre-existing, but I will clean it up.

Loading

Copy link
Member

@albertnetymk albertnetymk left a comment

Thank you for the revision.

Loading

@lkorinth
Copy link
Contributor Author

@lkorinth lkorinth commented May 19, 2021

/integrate

Loading

@openjdk openjdk bot closed this May 19, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2021

@lkorinth Since your change was applied there have been 18 commits pushed to the master branch:

  • 392f962: 8267151: C2: Don't create dummy Opaque1Node for outmost unswitched IfNode
  • 0cf7e57: 8267239: C1: RangeCheckElimination for % operator if divisor is IntConstant
  • 2d407e1: 8267293: vmTestbase/vm/mlvm/anonloader/stress/oome/metaspace/Test.java fails when JTREG_JOBS > 25
  • 70f6c67: 8233380: CHT: Node allocation and freeing
  • 2563a6a: 8266962: Add arch supporting check for "Op_VectorLoadConst" before creating the node
  • 4954383: 8267364: Remove mask.incr which is introduced by JDK-8256973
  • c2b50f9: 8266480: Implicit null check optimization does not update control of hoisted memory operation
  • 3f883e8: 8267351: runtime/cds/SharedBaseAddress.java fails on x86_32 due to Unrecognized VM option 'UseCompressedOops'
  • 7aa6568: 8256973: Intrinsic creation for VectorMask query (lastTrue,firstTrue,trueCount) APIs
  • 65a8bf5: 8265126: [REDO] unified handling for VectorMask object re-materialization during de-optimization
  • ... and 8 more: https://git.openjdk.java.net/jdk/compare/f8f40ab25ffce0243a40f307b0341cddd12ccb5d...master

Your commit was automatically rebased without conflicts.

Pushed as commit 6ef46ce.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants