Navigation Menu

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

8261137: Optimization of Box nodes in uncommon_trap #2401

Closed
wants to merge 13 commits into from
54 changes: 53 additions & 1 deletion src/hotspot/share/opto/callGenerator.cpp
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2000, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -554,6 +554,56 @@ void LateInlineVirtualCallGenerator::do_late_inline() {
CallGenerator::do_late_inline_helper();
}

// replace box node to scalar node only in case it is directly referenced by debug info
static void replace_box_to_scalar(CallNode* call, Node* resproj) {
if (resproj != nullptr && call->is_CallStaticJava() &&
call->as_CallStaticJava()->is_boxing_method()) {
Unique_Node_List debuginfo_node_list;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename this to safepoints.

Copy link
Author

Choose a reason for hiding this comment

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

Sure.

for (DUIterator_Fast imax, i = resproj->fast_outs(imax); i < imax; i++) {
Node* m = resproj->fast_out(i);
if (m->is_SafePoint()) {
SafePointNode* sfpt = m->as_SafePoint();
uint dbg_start = sfpt->is_Call() ? sfpt->as_Call()->tf()->domain()->cnt() : (uint)TypeFunc::Parms+1;
for (uint i = 0; i < dbg_start; i++) {
if (sfpt->in(i) == resproj) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

I think this code can be replaced by:

  if (!sfpt->is_Call() || !sfpt->as_Call()->has_non_debug_use(n)) {
    safepoints.push(sfpt);
  } else {
    ...
  }

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I will do that.

}
}
debuginfo_node_list.push(m);
} else {
return;
}
}

GraphKit kit(call->jvms());
PhaseGVN& gvn = kit.gvn();
// delay box in runtime, treat box as a scalarized object
while (debuginfo_node_list.size() > 0) {
ProjNode* res = resproj->as_Proj();
Node* debuginfo_node = debuginfo_node_list.pop();
Copy link
Member

Choose a reason for hiding this comment

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

debuginfo_node -> safepoint

Copy link
Author

Choose a reason for hiding this comment

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

OK.


ciInstanceKlass* klass = call->as_CallStaticJava()->method()->holder();
int n_fields = klass->nof_nonstatic_fields();
assert(n_fields == 1, "the klass must be an auto-boxing klass");
Copy link
Member

Choose a reason for hiding this comment

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

This code can be put in ifdef ASSERT and n_fields below can be replaced by 1.

Copy link
Author

Choose a reason for hiding this comment

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

These codes are similar to #853 (comment) , so I put assert here.


uint first_ind = (debuginfo_node->req() - debuginfo_node->jvms()->scloff());
Node* sobj = new SafePointScalarObjectNode(gvn.type(res)->isa_oopptr(),
#ifdef ASSERT
call->isa_Allocate(),

Choose a reason for hiding this comment

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

It is always NULL since call can't be an Allocate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was my suggestion to pass call node as allocation so that we could trace back for what node SafePointScalarObject was created because you may have several Box objects for which we create SafePointScalarObject nodes.
I think we can change argument (and field type) to CallNode. And have assert in SafePointScalarObject constructor to check that it is either Allocate node or Boxing Call node.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review. I will change the SafePointScalarObject constructor in next push.

#endif // ASSERT
first_ind, n_fields, true);
sobj->init_req(0, kit.root());
debuginfo_node->add_req(call->in(res->_con));
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you are selecting the input based on the result projection field res->_con?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review. I use sfpt->add_req(call->in(TypeFunc::Parms)); instead of this.

sobj = gvn.transform(sobj);
JVMState* jvms = debuginfo_node->jvms();
jvms->set_endoff(debuginfo_node->req());
int start = jvms->debug_start();
int end = jvms->debug_end();
debuginfo_node->replace_edges_in_range(res, sobj, start, end);
}
}
}

void CallGenerator::do_late_inline_helper() {
assert(is_late_inline(), "only late inline allowed");

Expand Down Expand Up @@ -603,6 +653,8 @@ void CallGenerator::do_late_inline_helper() {
C->remove_macro_node(call);
}

replace_box_to_scalar(call, callprojs.resproj);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be guarded by C->eliminate_boxing()?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly. I will change that.


bool result_not_used = (callprojs.resproj == NULL || callprojs.resproj->outcnt() == 0);
if (is_pure_call() && result_not_used) {
// The call is marked as pure (no important side effects), but result isn't used.
Expand Down
6 changes: 4 additions & 2 deletions src/hotspot/share/opto/callnode.cpp
Expand Up @@ -1512,10 +1512,12 @@ SafePointScalarObjectNode::SafePointScalarObjectNode(const TypeOopPtr* tp,
AllocateNode* alloc,
#endif
uint first_index,
uint n_fields) :
uint n_fields,
bool is_auto_box) :
TypeNode(tp, 1), // 1 control input -- seems required. Get from root.
_first_index(first_index),
_n_fields(n_fields)
_n_fields(n_fields),
_is_auto_box(is_auto_box)
#ifdef ASSERT
, _alloc(alloc)
#endif
Expand Down
4 changes: 3 additions & 1 deletion src/hotspot/share/opto/callnode.hpp
Expand Up @@ -500,6 +500,7 @@ class SafePointScalarObjectNode: public TypeNode {
// states of the scalarized object fields are collected.
// It is relative to the last (youngest) jvms->_scloff.
uint _n_fields; // Number of non-static fields of the scalarized object.
bool _is_auto_box; // is the scalarized object is auto box.
Copy link
Member

Choose a reason for hiding this comment

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

Typo in comment. Should be something like // True if the scalarized object is an auto box

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your review. I'll add this in my next patch.

DEBUG_ONLY(AllocateNode* _alloc;)

virtual uint hash() const ; // { return NO_HASH; }
Expand All @@ -512,7 +513,7 @@ class SafePointScalarObjectNode: public TypeNode {
#ifdef ASSERT
AllocateNode* alloc,
#endif
uint first_index, uint n_fields);
uint first_index, uint n_fields, bool is_auto_box = false);
virtual int Opcode() const;
virtual uint ideal_reg() const;
virtual const RegMask &in_RegMask(uint) const;
Expand All @@ -525,6 +526,7 @@ class SafePointScalarObjectNode: public TypeNode {
}
uint n_fields() const { return _n_fields; }

bool is_auto_box() const { return _is_auto_box; }
#ifdef ASSERT
AllocateNode* alloc() const { return _alloc; }
#endif
Expand Down
10 changes: 6 additions & 4 deletions src/hotspot/share/opto/output.cpp
Expand Up @@ -826,8 +826,9 @@ void PhaseOutput::FillLocArray( int idx, MachSafePointNode* sfpt, Node *local,
ciKlass* cik = t->is_oopptr()->klass();
assert(cik->is_instance_klass() ||
cik->is_array_klass(), "Not supported allocation.");
sv = new ObjectValue(spobj->_idx,
new ConstantOopWriteValue(cik->java_mirror()->constant_encoding()));
ScopeValue* klass_sv = new ConstantOopWriteValue(cik->java_mirror()->constant_encoding());
sv = spobj->is_auto_box() ? new AutoBoxObjectValue(spobj->_idx, klass_sv)
: new ObjectValue(spobj->_idx, klass_sv);
set_sv_for_object_node(objs, sv);

uint first_ind = spobj->first_index(sfpt->jvms());
Expand Down Expand Up @@ -1099,8 +1100,9 @@ void PhaseOutput::Process_OopMap_Node(MachNode *mach, int current_offset) {
ciKlass* cik = t->is_oopptr()->klass();
assert(cik->is_instance_klass() ||
cik->is_array_klass(), "Not supported allocation.");
ObjectValue* sv = new ObjectValue(spobj->_idx,
new ConstantOopWriteValue(cik->java_mirror()->constant_encoding()));
ScopeValue* klass_sv = new ConstantOopWriteValue(cik->java_mirror()->constant_encoding());
ObjectValue* sv = spobj->is_auto_box() ? new AutoBoxObjectValue(spobj->_idx, klass_sv)
: new ObjectValue(spobj->_idx, klass_sv);
PhaseOutput::set_sv_for_object_node(objs, sv);

uint first_ind = spobj->first_index(youngest_jvms);
Expand Down
15 changes: 7 additions & 8 deletions src/hotspot/share/runtime/deoptimization.cpp
Expand Up @@ -897,7 +897,7 @@ Deoptimization::DeoptAction Deoptimization::_unloaded_action



#if INCLUDE_JVMCI || INCLUDE_AOT
#if COMPILER2 || INCLUDE_JVMCI || INCLUDE_AOT
template<typename CacheType>
class BoxCacheBase : public CHeapObj<mtCompiler> {
protected:
Expand Down Expand Up @@ -1026,7 +1026,7 @@ oop Deoptimization::get_cached_box(AutoBoxObjectValue* bv, frame* fr, RegisterMa
}
return NULL;
}
#endif // INCLUDE_JVMCI || INCLUDE_AOT
#endif // COMPILER2 || INCLUDE_JVMCI || INCLUDE_AOT

#if COMPILER2_OR_JVMCI
bool Deoptimization::realloc_objects(JavaThread* thread, frame* fr, RegisterMap* reg_map, GrowableArray<ScopeValue*>* objects, TRAPS) {
Expand All @@ -1045,17 +1045,16 @@ bool Deoptimization::realloc_objects(JavaThread* thread, frame* fr, RegisterMap*
oop obj = NULL;

if (k->is_instance_klass()) {
#if INCLUDE_JVMCI || INCLUDE_AOT
CompiledMethod* cm = fr->cb()->as_compiled_method_or_null();
if (cm->is_compiled_by_jvmci() && sv->is_auto_box()) {
#if COMPILER2 || INCLUDE_JVMCI || INCLUDE_AOT
if (sv->is_auto_box()) {
AutoBoxObjectValue* abv = (AutoBoxObjectValue*) sv;
obj = get_cached_box(abv, fr, reg_map, THREAD);
if (obj != NULL) {
// Set the flag to indicate the box came from a cache, so that we can skip the field reassignment for it.
abv->set_cached(true);
}
}
#endif // INCLUDE_JVMCI || INCLUDE_AOT
#endif // COMPILER2 || INCLUDE_JVMCI || INCLUDE_AOT
InstanceKlass* ik = InstanceKlass::cast(k);
if (obj == NULL) {
#ifdef COMPILER2
Expand Down Expand Up @@ -1397,12 +1396,12 @@ void Deoptimization::reassign_fields(frame* fr, RegisterMap* reg_map, GrowableAr
if (obj.is_null()) {
continue;
}
#if INCLUDE_JVMCI || INCLUDE_AOT
#if COMPILER2 || INCLUDE_JVMCI || INCLUDE_AOT
// Don't reassign fields of boxes that came from a cache. Caches may be in CDS.
if (sv->is_auto_box() && ((AutoBoxObjectValue*) sv)->is_cached()) {
continue;
}
#endif // INCLUDE_JVMCI || INCLUDE_AOT
#endif // COMPILER2 || INCLUDE_JVMCI || INCLUDE_AOT
#ifdef COMPILER2
if (EnableVectorSupport && VectorSupport::is_vector(k)) {
continue; // skip field reassignment for vectors
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/runtime/deoptimization.hpp
Expand Up @@ -163,9 +163,10 @@ class Deoptimization : AllStatic {

#if INCLUDE_JVMCI
static address deoptimize_for_missing_exception_handler(CompiledMethod* cm);
static oop get_cached_box(AutoBoxObjectValue* bv, frame* fr, RegisterMap* reg_map, TRAPS);
#endif

static oop get_cached_box(AutoBoxObjectValue* bv, frame* fr, RegisterMap* reg_map, TRAPS);

private:
// Does the actual work for deoptimizing a single frame
static void deoptimize_single_frame(JavaThread* thread, frame fr, DeoptReason reason);
Expand Down