Permalink
Browse files

Avoid extra references in inout wrappers

Summary:
This diff makes a few performance tweaks to the inout parameter implementation:
 - Don't verify return typehints twice when calling through wrapper functions
 - For wrappers pass all by-value non-ref, non-inout parameters via PushL to avoid observing an extra reference count
 - For wrappers set ref arguments to Null after pushing their values to the stack to avoid observing an extra reference count via the ref
 - For each inout parameter at a callsite if (1) the callsite isn't within a try block, and (2) the parameter is only seen once in the course of the call, set it to Null after pushing to the stack to avoid observing an extra reference count.

The setting of inout parameters and reffy arguments to null creates an observable behavioral change in the presence of references. Since this behavioral quirk is restricted to inout functions, which themselves aim to eliminate the need for references this is a temporary and hopefully workable solution to some of the performance issues associated with these wrappers.

Reviewed By: swtaarrs

Differential Revision: D6776155

fbshipit-source-id: 93bf273d46a06d68a1cfa6c699c25234d8222e06
  • Loading branch information...
paulbiss authored and hhvm-bot committed Jan 27, 2018
1 parent 5a529f2 commit 975060a6583e9c200e55b048dc3ef7eb095af798
@@ -962,6 +962,9 @@ struct EmitterVisitor {
// stack of nested unnamed pipe variables
std::stack<Id> m_pipeVars;
// Set if currently inside a try block
bool m_inTry{false};
public:
bool checkIfStackEmpty(const char* forInstruction) const;
void unexpectedStackSym(char sym, const char* where) const;
@@ -1041,7 +1044,8 @@ struct EmitterVisitor {
bool isInChain(const MInstrChain& chain, int off);
MInstrChain emitInOutArg(Emitter& e, ExpressionPtr exp, int paramId,
LocalAliasMap* vars);
LocalAliasMap* vars,
std::unordered_map<Id, uint64_t>& seen);
int scanStackForLocation(int iLast);
@@ -1114,7 +1118,7 @@ struct EmitterVisitor {
void emitConvertSecondToCell(Emitter& e);
void emitConvertToVar(Emitter& e);
void emitFPass(Emitter& e, int paramID, PassByRefKind passByRefKind,
FPassHint hint, bool isInOut = false);
FPassHint hint, bool isInOut = false, bool canMove = false);
Offset emitFPushClsMethod(Emitter& e, ExpressionListPtr params);
void emitVirtualLocal(int localId);
template<class Expr> void emitVirtualClassBase(Emitter&, Expr* node);
@@ -3003,7 +3007,13 @@ bool EmitterVisitor::emitTryCatch(
);
}
visit(body);
{
auto const prevTry = m_inTry;
m_inTry = true;
SCOPE_EXIT { m_inTry = prevTry; };
visit(body);
}
auto const try_end = m_ue.bcPos();
if (!m_evalStack.empty()) {
@@ -4565,7 +4575,8 @@ static void collect_written_variables(
ConstructPtr node,
int param_id,
FuncEmitter* fe,
EmitterVisitor::LocalAliasMap& map
EmitterVisitor::LocalAliasMap& map,
std::unordered_map<Id, uint64_t>& seen
) {
if (!node) return;
@@ -4575,14 +4586,15 @@ static void collect_written_variables(
Expression::InOutParameter |
Expression::RefParameter
);
if (is_write && !sv->isThis()) {
if (!sv->isThis() && !sv->isSuperGlobal()) {
auto const var_id = fe->lookupVarId(makeStaticString(sv->getName()));
map[var_id].write(param_id);
if (is_write) map[var_id].write(param_id);
seen[var_id]++;
}
return;
}
for (int i = 0; i < node->getKidCount(); ++i) {
collect_written_variables(node->getNthKid(i), param_id, fe, map);
collect_written_variables(node->getNthKid(i), param_id, fe, map, seen);
}
}
@@ -4605,6 +4617,7 @@ void EmitterVisitor::emitCall(Emitter& e,
}();
std::vector<MInstrChain> chains;
std::unordered_map<Id, uint64_t> seen;
LocalAliasMap varMap;
if (anyInOut) {
@@ -4621,9 +4634,10 @@ void EmitterVisitor::emitCall(Emitter& e,
} else if (sv->hasContext(Expression::RefParameter)) {
varMap[id].write(i);
}
seen[id]++;
}
} else {
collect_written_variables(param, i, m_curFunc, varMap);
collect_written_variables(param, i, m_curFunc, varMap, seen);
}
}
}
@@ -4637,9 +4651,9 @@ void EmitterVisitor::emitCall(Emitter& e,
} else if (dynamic_pointer_cast<SimpleVariable>(param)) {
// If the param writes directly to a local then it doesn't need to be
// shadowed. If it gets shadowed that's fine, the final write will win.
chains.push_back(emitInOutArg(e, param, i, nullptr));
chains.push_back(emitInOutArg(e, param, i, nullptr, seen));
} else {
chains.push_back(emitInOutArg(e, param, i, &varMap));
chains.push_back(emitInOutArg(e, param, i, &varMap, seen));
}
}
}
@@ -7850,7 +7864,8 @@ EmitterVisitor::MInstrChain EmitterVisitor::emitInOutArg(
Emitter& e,
ExpressionPtr exp,
int paramId,
LocalAliasMap* vars
LocalAliasMap* vars,
std::unordered_map<Id, uint64_t>& seen
) {
m_minstrChains.push_back(
MInstrChain{vars, paramId, m_evalStack.size()}
@@ -7872,14 +7887,29 @@ EmitterVisitor::MInstrChain EmitterVisitor::emitInOutArg(
);
}
emitFPass(e, paramId, getPassByRefKind(exp), getPassByRefHint(exp), true);
auto const canMove = [&] {
if (auto sv = dynamic_pointer_cast<SimpleVariable>(exp)) {
if (!sv->isThis() && !sv->isSuperGlobal()) {
auto const var_id =
m_curFunc->lookupVarId(makeStaticString(sv->getName()));
// If more than one reference to this parameter exists in the argument
// list then it isn't safe to "move" it by unsetting the local.
return seen[var_id] < 2;
}
}
return false;
}();
emitFPass(e, paramId, getPassByRefKind(exp), getPassByRefHint(exp), true,
canMove);
return chain;
}
void EmitterVisitor::emitFPass(Emitter& e, int paramId,
PassByRefKind passByRefKind,
FPassHint hint,
bool isInOut) {
bool isInOut,
bool canMove) {
if (checkIfStackEmpty("FPass*")) return;
LocationGuard locGuard(e, m_tempLoc);
m_tempLoc.clear();
@@ -7898,7 +7928,13 @@ void EmitterVisitor::emitFPass(Emitter& e, int paramId,
"Parameters marked inout must be contained in locals, vecs, dicts, "
"keysets, and arrays");
}
e.CGetL(m_evalStack.getLoc(i));
auto const loc = m_evalStack.getLoc(i);
emitCGet(e);
if (!m_inTry && canMove) {
emitVirtualLocal(loc);
e.Null();
e.PopL(loc);
}
e.FPassC(paramId, hint);
return;
}
@@ -9019,9 +9055,10 @@ void EmitterVisitor::emitFuncWrapper(PostponedMeth& p, FuncEmitter* fe,
}
{
fe->retTypeConstraint = TypeConstraint{};
FPIRegionRecorder fpi(this, m_ue, m_evalStack, fpiStart);
for (int i = 0; i < fe->params.size(); i++) {
emitVirtualLocal(i);
fe->params[i].typeConstraint = TypeConstraint{};
getParam(e, i);
}
}
@@ -9051,11 +9088,13 @@ void EmitterVisitor::emitInOutToRefWrapper(PostponedMeth& p,
fe->params[i].byRef = false;
fe->params[i].inout = true;
inoutParams.push_back(i);
emitVirtualLocal(i);
emitVGet(e);
e.FPassVNop(i, FPassHint::Ref);
e.FPassVNop(i, FPassHint::Any);
} else {
emitCGetQuiet(e);
e.FPassC(i, FPassHint::Cell);
emitVirtualLocal(i);
e.PushL(i);
e.FPassC(i, FPassHint::Any);
}
},
[&] (Emitter& e) {
@@ -9082,19 +9121,24 @@ void EmitterVisitor::emitRefToInOutWrapper(PostponedMeth& p,
fe->params[i].byRef = true;
fe->params[i].inout = false;
refParams.push_back(i);
emitVirtualLocal(i);
emitCGetQuiet(e);
// Set the bound local to null to drop a reference to it
emitVirtualLocal(i);
e.Null();
e.PopL(i);
} else {
emitVirtualLocal(i);
e.PushL(i);
}
emitCGetQuiet(e);
e.FPassC(i, FPassHint::Cell);
e.FPassC(i, FPassHint::Any);
},
[&] (Emitter& e) {
emitUnpackInOutCall(
e,
refParams.size(),
[&] (uint32_t i) { emitVirtualLocal(refParams[i]); }
);
if (shouldEmitVerifyRetType()) {
e.VerifyRetTypeC();
}
},
false
);
@@ -848,6 +848,7 @@ let makeunaryinst s arg = match s with
(* instruct_mutator *)
| "SetL" -> IMutator (SetL (localidofiarg arg))
| "PopL" -> IMutator (PopL (localidofiarg arg))
| "SetS" -> IMutator (SetS (intofiarg arg))
| "SetOpN" -> IMutator(SetOpN (eqopofiarg arg))
| "SetOpG" -> IMutator(SetOpG (eqopofiarg arg))
@@ -13,7 +13,8 @@ type t = {
env_scope : Ast_scope.Scope.t;
env_namespace : Namespace_env.env;
env_needs_local_this: bool;
env_jump_targets : Jump_targets.t
env_jump_targets : Jump_targets.t;
env_in_try : bool
}
type global_state =
@@ -68,13 +69,15 @@ let empty = {
env_namespace = Namespace_env.empty_with_default_popt;
env_needs_local_this = false;
env_jump_targets = Jump_targets.empty;
env_in_try = false;
}
let get_pipe_var env = env.env_pipe_var
let get_scope env = env.env_scope
let get_namespace env = env.env_namespace
let get_needs_local_this env = env.env_needs_local_this
let get_jump_targets env = env.env_jump_targets
let is_in_try env = env.env_in_try
(* Environment is second parameter so we can chain these e.g.
* empty |> with_scope scope |> with_namespace ns
@@ -90,7 +93,8 @@ let with_pipe_var v env =
let make_class_env ast_class =
{ env_pipe_var = None; env_scope = [Ast_scope.ScopeItem.Class ast_class];
env_namespace = ast_class.Ast.c_namespace; env_needs_local_this = false;
env_jump_targets = Jump_targets.empty; }
env_jump_targets = Jump_targets.empty; env_in_try = false; }
let with_try env = { env with env_in_try = true }
let do_in_loop_body break_label continue_label ?iter env s f =
Jump_targets.with_loop (!is_hh_file_) break_label continue_label
@@ -49,18 +49,23 @@ module InoutLocals = struct
type alias_info = {
first_inout: int;
last_write: int;
num_uses: int;
}
let not_aliased =
{ first_inout = max_int; last_write = min_int }
{ first_inout = max_int; last_write = min_int; num_uses = 0; }
let add_inout i r =
if i < r.first_inout then { r with first_inout = i } else r
let add_write i r =
if i > r.last_write then { r with last_write = i } else r
let add_use _i r =
{ r with num_uses = r.num_uses + 1 }
let in_range i r = i > r.first_inout || i <= r.last_write
let has_single_ref r = r.num_uses < 2
let update name i f m =
let r =
@@ -71,6 +76,7 @@ module InoutLocals = struct
let add_write name i m = update name i add_write m
let add_inout name i m = update name i add_inout m
let add_use name i m = update name i add_use m
let collect_written_variables env args =
(* check value of the argument *)
@@ -79,12 +85,15 @@ module InoutLocals = struct
(* inout $v *)
| A.Callconv (A.Pinout, (_, A.Lvar (_, id)))
when not (is_local_this env id) ->
let acc = add_use id i acc in
if is_top then add_inout id i acc else add_write id i acc
(* &$v *)
| A.Unop (A.Uref, (_, A.Lvar (_, id))) ->
let acc = add_use id i acc in
add_write id i acc
(* $v *)
| A.Lvar _ ->
| A.Lvar (_, id) ->
let acc = add_use id i acc in
acc
| _ ->
(* dive into argument value *)
@@ -94,6 +103,7 @@ module InoutLocals = struct
and collect_lvars_lhs i acc e =
match snd e with
| A.Lvar (_, id) when not (is_local_this env id) ->
let acc = add_use id i acc in
add_write id i acc
| A.List exprs ->
List.fold_left exprs ~f:(collect_lvars_lhs i) ~init:acc
@@ -130,6 +140,8 @@ module InoutLocals = struct
should be saved to local because it might be overwritten later *)
let should_save_local_value name i aliases =
Option.value_map ~default:false ~f:(in_range i) (SMap.get name aliases)
let should_move_local_value name aliases =
Option.value_map ~default:true ~f:has_single_ref (SMap.get name aliases)
end
(* Describes what kind of value is intended to be stored in local *)
@@ -2434,11 +2446,17 @@ and emit_args_and_call env call_pos args uargs =
instr_string (SU.Locals.strip_dollar x);
instr_fpassg i hint;
]
| A.Lvar (_, s) when is_inout ->
| A.Lvar ((_, s) as id) when is_inout ->
let inout_setters =
(instr_setl @@ Local.Named s) :: inout_setters in
let not_in_try = not (Emit_env.is_in_try env) in
let move_instrs =
if not_in_try && (InoutLocals.should_move_local_value s aliases)
then gather [ instr_null; instr_popl (get_local env id) ]
else empty in
gather [
emit_expr ~need_ref:false env expr;
move_instrs;
Emit_pos.emit_pos call_pos;
instr_fpassc i hint;
aux (i + 1) rest inout_setters
Oops, something went wrong.

0 comments on commit 975060a

Please sign in to comment.