From f1231364460465b545a4acd97f4cab3fabd5056b Mon Sep 17 00:00:00 2001 From: Shaunak Kishore Date: Fri, 18 Dec 2020 23:00:58 -0800 Subject: [PATCH] Drop native Shapes::idx 'optimization' Summary: The only purpose of making Shapes::idx a native call was to force it to be inlined via FCallBuiltin so that we could use the offset lookup optimization for these array accesses. Now that FCallBuiltin is dead, we'll never do this profiling. That means a pure Hack implementation will work as well as the builtin. Reviewed By: oulgen Differential Revision: D25640887 fbshipit-source-id: 823edcae1cb7c4ddb569637c7ba15f8fb6577e2c --- hphp/hhbbc/interp-builtin.cpp | 57 --------------- hphp/runtime/ext/shapes/config.cmake | 6 -- hphp/runtime/ext/shapes/ext_shapes.cpp | 80 ---------------------- hphp/runtime/vm/jit/irgen-builtin.cpp | 58 ---------------- hphp/system/php.txt | 1 + .../ext => system/php}/shapes/ext_shapes.php | 6 +- .../class-ptr/classptr-eager-convert.php.expectf | 4 +- .../class-ptr/classptr-eager-convert.php.hphp_opts | 1 + .../slow/class-ptr/classptr-eager-convert.php.opts | 1 + .../class-ptr/lazyclass-convert-key.php.expectf | 4 +- .../class-ptr/lazyclass-convert-key.php.hphp_opts | 1 + .../slow/class-ptr/lazyclass-convert-key.php.opts | 1 + hphp/test/slow/hack_arr_compat/specialization.php | 2 +- .../hack_arr_compat/specialization.php.expectf | 2 +- hphp/test/slow/shapes/idx_arr_compat.php.expectf | 8 +-- .../idx_arr_null_still_checks_arraykey.php.expectf | 2 +- .../slow/shapes/idx_arr_type_error.php.expectf | 4 +- .../slow/shapes/idx_key_type_error.php.expectf | 2 +- hphp/util/hphp-config.h.in | 3 +- 19 files changed, 21 insertions(+), 222 deletions(-) delete mode 100644 hphp/runtime/ext/shapes/config.cmake delete mode 100644 hphp/runtime/ext/shapes/ext_shapes.cpp rename hphp/{runtime/ext => system/php}/shapes/ext_shapes.php (91%) diff --git a/hphp/hhbbc/interp-builtin.cpp b/hphp/hhbbc/interp-builtin.cpp index 8e7f780280f..517564eace6 100644 --- a/hphp/hhbbc/interp-builtin.cpp +++ b/hphp/hhbbc/interp-builtin.cpp @@ -394,62 +394,6 @@ TypeOrReduced builtin_type_structure_classname(ISS& env, const php::Func* func, return TSStr; } -TypeOrReduced builtin_shapes_idx(ISS& env, const php::Func* func, - const FCallArgs& fca) { - assertx(fca.numArgs() >= 2 && fca.numArgs() <= 3); - auto def = to_cell(getArg(env, func, fca, 2)); - auto const key = getArg(env, func, fca, 1); - auto const base = getArg(env, func, fca, 0); - const auto optDArr = RuntimeOption::EvalHackArrDVArrs ? BOptDict : BOptArr; - - if (!base.couldBe(optDArr) || - !key.couldBe(BArrKeyCompat)) { - unreachable(env); - return TBottom; - } - if (!base.subtypeOf(optDArr) || !key.subtypeOf(BOptArrKeyCompat)) { - return NoReduced{}; - } - - auto mightThrow = is_opt(key); - - if (base.subtypeOf(BNull)) { - if (!mightThrow) { - constprop(env); - effect_free(env); - } - return std::move(def); - } - - auto const unoptBase = is_opt(base) ? unopt(base) : base; - auto const unoptKey = is_opt(key) ? unopt(key) : key; - - auto elem = RuntimeOption::EvalHackArrDVArrs - ? dictish_elem(unoptBase, unoptKey, def) - : array_like_elem(unoptBase, unoptKey, def, true); - switch (elem.second) { - case ThrowMode::None: - case ThrowMode::MaybeMissingElement: - case ThrowMode::MissingElement: - break; - case ThrowMode::MaybeBadKey: - mightThrow = true; - break; - case ThrowMode::BadOperation: - always_assert(false); - } - - if (!mightThrow) { - constprop(env); - effect_free(env); - } - - auto res = elem.first; - if (is_opt(base)) res |= def; - - return std::move(res); -} - #define SPECIAL_BUILTINS \ X(abs, abs) \ X(ceil, ceil) \ @@ -468,7 +412,6 @@ TypeOrReduced builtin_shapes_idx(ISS& env, const php::Func* func, X(type_structure, HH\\type_structure) \ X(type_structure_no_throw, HH\\type_structure_no_throw) \ X(type_structure_classname, HH\\type_structure_classname) \ - X(shapes_idx, HH\\Shapes::idx) \ #define X(x, y) const StaticString s_##x(#y); SPECIAL_BUILTINS diff --git a/hphp/runtime/ext/shapes/config.cmake b/hphp/runtime/ext/shapes/config.cmake deleted file mode 100644 index e072362695d..00000000000 --- a/hphp/runtime/ext/shapes/config.cmake +++ /dev/null @@ -1,6 +0,0 @@ -HHVM_DEFINE_EXTENSION("shapes" REQUIRED - SOURCES - ext_shapes.cpp - SYSTEMLIB - ext_shapes.php -) diff --git a/hphp/runtime/ext/shapes/ext_shapes.cpp b/hphp/runtime/ext/shapes/ext_shapes.cpp deleted file mode 100644 index a20a7991ba2..00000000000 --- a/hphp/runtime/ext/shapes/ext_shapes.cpp +++ /dev/null @@ -1,80 +0,0 @@ -/* - +----------------------------------------------------------------------+ - | HipHop for PHP | - +----------------------------------------------------------------------+ - | Copyright (c) 2010-present Facebook, Inc. (http://www.facebook.com) | - | Copyright (c) 1997-2010 The PHP Group | - +----------------------------------------------------------------------+ - | This source file is subject to version 3.01 of the PHP license, | - | that is bundled with this package in the file LICENSE, and is | - | available through the world-wide-web at the following url: | - | http://www.php.net/license/3_01.txt | - | If you did not receive a copy of the PHP license and are unable to | - | obtain it through the world-wide-web, please send a note to | - | license@php.net so we can mail you a copy immediately. | - +----------------------------------------------------------------------+ -*/ - -#include "hphp/runtime/base/builtin-functions.h" -#include "hphp/runtime/base/datatype.h" -#include "hphp/runtime/base/tv-val.h" -#include "hphp/runtime/base/typed-value.h" -#include "hphp/runtime/ext/extension.h" -#include "hphp/runtime/ext/std/ext_std_misc.h" -#include "hphp/runtime/vm/bytecode.h" -#include "hphp/system/systemlib.h" - -namespace HPHP { -namespace { - -/////////////////////////////////////////////////////////////////////////////// - -void raiseExpectedArrayKey(TypedValue key) { - raise_error(folly::sformat( - "Argument 2 passed to HH\\Shapes::idx() " - "must be an instance of arraykey, {} given", - getDataTypeString(type(key)).data() - )); - not_reached(); -} - -TypedValue shapes_idx(const Class* self, const Variant& maybe_shape, - TypedValue key, TypedValue def /*= uninit */) { - if (maybe_shape.isNull()) { - // still check second arg type - if (!(isIntType(type(key)) || isStringType(type(key)) || - isClassType(type(key)) || isLazyClassType(type(key)))) { - raiseExpectedArrayKey(key); - not_reached(); - } - return tvReturn(tvAsCVarRef(&def)); - } - - const Array& shape = maybe_shape.asCArrRef(); - key = tvClassToString(key); - if (isIntType(type(key))) { - auto const result = shape->get(val(key).num); - if (result.is_init()) return tvReturn(tvAsCVarRef(result)); - } else if (isStringType(type(key))) { - auto const result = shape->get(val(key).pstr); - if (result.is_init()) return tvReturn(tvAsCVarRef(result)); - } else { - raiseExpectedArrayKey(key); - not_reached(); - } - return tvReturn(tvAsCVarRef(&def)); -} - -static struct ShapesExtension final : Extension { - ShapesExtension() : Extension("shapes") { } - - void moduleInit() override { - HHVM_NAMED_STATIC_ME(HH\\Shapes, idx, shapes_idx); - loadSystemlib("shapes"); - } -} s_shapes_extension; - -/////////////////////////////////////////////////////////////////////////////// - -} // namespace -} // namespace HPHP diff --git a/hphp/runtime/vm/jit/irgen-builtin.cpp b/hphp/runtime/vm/jit/irgen-builtin.cpp index 846e84bb7e9..6d4a3ee1d39 100644 --- a/hphp/runtime/vm/jit/irgen-builtin.cpp +++ b/hphp/runtime/vm/jit/irgen-builtin.cpp @@ -1031,62 +1031,6 @@ SSATmp* opt_class_meth_get_method(IRGS& env, const ParamPrep& params) { return nullptr; } -SSATmp* opt_shapes_idx(IRGS& env, const ParamPrep& params) { - // We first check the number and types of each argument. If any check fails, - // we'll fall back to the native code which will raise an appropriate error. - auto const nparams = params.size(); - if (nparams != 2 && nparams != 3) return nullptr; - - // params[1] is an arraykey. We only optimize if it's narrowed to int or str. - auto const keyType = params[1].value->type(); - if (!(keyType <= TInt || keyType <= TStr)) return nullptr; - - // params[2] is an optional argument. If it's uninit, we convert it to null. - // We only optimize if we can distinguish between uninit and other types. - auto const defType = nparams == 3 ? params[2].value->type() : TUninit; - if (!(defType <= TUninit) && defType.maybe(TUninit)) return nullptr; - auto const def = defType <= TUninit ? cns(env, TInitNull) : params[2].value; - - // params[0] is a ?darray, which may be a ?Dict or a ?DArr based on options. - auto const arrType = params[0].value->type(); - if (!(RuntimeOption::EvalHackArrDVArrs ? - arrType <= TDict : arrType <= TDArr)) { - if (arrType <= TNull) { - gen(env, IncRef, def); - return def; - } else { - return nullptr; - } - } - - // Do the array access, using array offset profiling to optimize it. - auto const arr = params[0].value; - auto const key = params[1].value; - // we might side-exit in profiledArrayAccess - env.irb->fs().incBCSPDepth(nparams); - auto const elm = profiledArrayAccess( - env, arr, key, MOpMode::None, - [&] (SSATmp* arr, SSATmp* key, SSATmp* pos) { - return gen(env, DictGetK, arr, key, pos); - }, - [&] (SSATmp*) { return def; }, - [&] (SSATmp* key, SizeHintData data) { - return gen(env, DictIdx, data, arr, key, def); - } - ); - env.irb->fs().decBCSPDepth(nparams); - - auto const finish = [&](SSATmp* val){ - gen(env, IncRef, val); - return val; - }; - return finish(profiledType(env, elm, [&] { - auto const cell = finish(elm); - params.decRefParams(env); - push(env, cell); - })); -} - const EnumValues* getEnumValues(IRGS& env, const ParamPrep& params) { if (RO::EvalArrayProvenance) return nullptr; if (!(params.ctx && params.ctx->hasConstVal(TCls))) return nullptr; @@ -1214,7 +1158,6 @@ const hphp_fast_string_imap s_opt_emit_fns{ {"HH\\class_meth_get_class", opt_class_meth_get_class}, {"HH\\class_meth_get_method", opt_class_meth_get_method}, {"HH\\class_get_class_name", opt_class_get_class_name}, - {"HH\\Shapes::idx", opt_shapes_idx}, {"HH\\BuiltinEnum::getNames", opt_enum_names}, {"HH\\BuiltinEnum::getValues", opt_enum_values}, {"HH\\BuiltinEnum::coerce", opt_enum_coerce}, @@ -1228,7 +1171,6 @@ const hphp_fast_string_imap s_opt_emit_fns{ // (if any) we need a vanilla input for to generate optimized HHIR. const hphp_fast_string_imap s_vanilla_params{ - {"HH\\Shapes::idx", 0}, {"HH\\Lib\\_Private\\Native\\first", 0}, {"HH\\Lib\\_Private\\Native\\last", 0}, {"HH\\Lib\\_Private\\Native\\first_key", 0}, diff --git a/hphp/system/php.txt b/hphp/system/php.txt index 7fbba31638b..0fc557ba601 100644 --- a/hphp/system/php.txt +++ b/hphp/system/php.txt @@ -104,6 +104,7 @@ hphp/system/php/misc/fb_autoload_map.php hphp/system/php/misc/idx.php hphp/system/php/pdo/PDOException.php hphp/system/php/rx/mutable.php +hphp/system/php/shapes/ext_shapes.php hphp/system/php/soap/SoapFault.php hphp/system/php/spl/datastructures/SplPriorityQueue.php hphp/system/php/spl/interfaces/SplObserver.php diff --git a/hphp/runtime/ext/shapes/ext_shapes.php b/hphp/system/php/shapes/ext_shapes.php similarity index 91% rename from hphp/runtime/ext/shapes/ext_shapes.php rename to hphp/system/php/shapes/ext_shapes.php index 22fae2b1d74..39ffc4d2fec 100644 --- a/hphp/runtime/ext/shapes/ext_shapes.php +++ b/hphp/system/php/shapes/ext_shapes.php @@ -3,12 +3,14 @@ namespace HH { abstract final class Shapes { - <<__Native, __Pure, __IsFoldable>> + <<__Pure, __IsFoldable>> public static function idx( ?darray $shape, arraykey $index, mixed $default = null, - ): mixed; + ) { + return idx($shape, $index, $default); + } <<__Pure>> public static function at( diff --git a/hphp/test/slow/class-ptr/classptr-eager-convert.php.expectf b/hphp/test/slow/class-ptr/classptr-eager-convert.php.expectf index 70770f7d093..18f2f44497b 100644 --- a/hphp/test/slow/class-ptr/classptr-eager-convert.php.expectf +++ b/hphp/test/slow/class-ptr/classptr-eager-convert.php.expectf @@ -133,5 +133,5 @@ darray(3) { Warning: Class to string conversion in %s/test/slow/class-ptr/class-key-convert.inc on line 96 int(3) -Warning: Class to string conversion in %s/test/slow/class-ptr/class-key-convert.inc on line 97 -int(3) \ No newline at end of file +Notice: Implicit Class to string conversion for type-hint in %s/test/slow/class-ptr/class-key-convert.inc on line 97 +int(3) diff --git a/hphp/test/slow/class-ptr/classptr-eager-convert.php.hphp_opts b/hphp/test/slow/class-ptr/classptr-eager-convert.php.hphp_opts index b666c537769..ed199d21fb2 100644 --- a/hphp/test/slow/class-ptr/classptr-eager-convert.php.hphp_opts +++ b/hphp/test/slow/class-ptr/classptr-eager-convert.php.hphp_opts @@ -1,2 +1,3 @@ -vRuntime.Eval.EmitClassPointers=1 -vRuntime.Eval.RaiseClassConversionWarning=true +-vRuntime.Eval.ClassStringHintNotices=true diff --git a/hphp/test/slow/class-ptr/classptr-eager-convert.php.opts b/hphp/test/slow/class-ptr/classptr-eager-convert.php.opts index 073888a912a..53b83cd608b 100644 --- a/hphp/test/slow/class-ptr/classptr-eager-convert.php.opts +++ b/hphp/test/slow/class-ptr/classptr-eager-convert.php.opts @@ -1,3 +1,4 @@ -vEval.EmitClassPointers=1 -vEval.RaiseClassConversionWarning=true -vEval.ClassAsStringVarDump=false +-vEval.ClassStringHintNotices=true diff --git a/hphp/test/slow/class-ptr/lazyclass-convert-key.php.expectf b/hphp/test/slow/class-ptr/lazyclass-convert-key.php.expectf index 9cd4eadd27e..ae4cbd1b4c2 100644 --- a/hphp/test/slow/class-ptr/lazyclass-convert-key.php.expectf +++ b/hphp/test/slow/class-ptr/lazyclass-convert-key.php.expectf @@ -125,5 +125,5 @@ darray(3) { Warning: Class to string conversion in %s/test/slow/class-ptr/class-key-convert.inc on line 96 int(3) -Warning: Class to string conversion in %s/test/slow/class-ptr/class-key-convert.inc on line 97 -int(3) \ No newline at end of file +Notice: Implicit Class to string conversion for type-hint in %s/test/slow/class-ptr/class-key-convert.inc on line 97 +int(3) diff --git a/hphp/test/slow/class-ptr/lazyclass-convert-key.php.hphp_opts b/hphp/test/slow/class-ptr/lazyclass-convert-key.php.hphp_opts index 838b20276b4..885f5c9f5d5 100644 --- a/hphp/test/slow/class-ptr/lazyclass-convert-key.php.hphp_opts +++ b/hphp/test/slow/class-ptr/lazyclass-convert-key.php.hphp_opts @@ -1,2 +1,3 @@ -vRuntime.Eval.EmitClassPointers=2 -vRuntime.Eval.RaiseClassConversionWarning=true +-vRuntime.Eval.ClassStringHintNotices=true diff --git a/hphp/test/slow/class-ptr/lazyclass-convert-key.php.opts b/hphp/test/slow/class-ptr/lazyclass-convert-key.php.opts index ebe72ac6f01..29914130777 100644 --- a/hphp/test/slow/class-ptr/lazyclass-convert-key.php.opts +++ b/hphp/test/slow/class-ptr/lazyclass-convert-key.php.opts @@ -1,3 +1,4 @@ -vEval.EmitClassPointers=2 -vEval.RaiseClassConversionWarning=true -vEval.ClassAsStringVarDump=false +-vEval.ClassStringHintNotices=true diff --git a/hphp/test/slow/hack_arr_compat/specialization.php b/hphp/test/slow/hack_arr_compat/specialization.php index 27d0a40ec38..3ef1f922be6 100644 --- a/hphp/test/slow/hack_arr_compat/specialization.php +++ b/hphp/test/slow/hack_arr_compat/specialization.php @@ -52,7 +52,7 @@ function test_as_is_shape_tuple() { function test_builtin_error_messages() { print("\n=====================================\nBuiltin errors:\n"); print('Passing boolean to darray: '); - run(() ==> Shapes::idx(false, 17)); + run(() ==> __hhvm_intrinsics\dummy_darray_builtin(false)); print('Passing darray to boolean: '); run(() ==> json_decode('[]', darray[])); } diff --git a/hphp/test/slow/hack_arr_compat/specialization.php.expectf b/hphp/test/slow/hack_arr_compat/specialization.php.expectf index 11f25dd1e21..4bdefb94867 100644 --- a/hphp/test/slow/hack_arr_compat/specialization.php.expectf +++ b/hphp/test/slow/hack_arr_compat/specialization.php.expectf @@ -13,7 +13,7 @@ darray is tuple: false ===================================== Builtin errors: -Passing boolean to darray: idx() expects parameter 1 to be darray, boolean given +Passing boolean to darray: __hhvm_intrinsics\dummy_darray_builtin() expects parameter 1 to be darray, boolean given Passing darray to boolean: json_decode() expects parameter 2 to be boolean, darray given ===================================== diff --git a/hphp/test/slow/shapes/idx_arr_compat.php.expectf b/hphp/test/slow/shapes/idx_arr_compat.php.expectf index d500bfb85c1..328dfb6bad8 100644 --- a/hphp/test/slow/shapes/idx_arr_compat.php.expectf +++ b/hphp/test/slow/shapes/idx_arr_compat.php.expectf @@ -12,9 +12,5 @@ varray(2) { bool(true) bool(false) } -string(52) "idx() expects parameter 1 to be darray, varray given" -object(HH\Map) (1) { - ["x"]=> - int(1) -} -string(52) "idx() expects parameter 1 to be darray, object given" + +Catchable fatal error: Argument 1 to HH\Shapes::idx() must be of type ?HH\darray, varray given in %s/idx_arr_compat.php on line 14 diff --git a/hphp/test/slow/shapes/idx_arr_null_still_checks_arraykey.php.expectf b/hphp/test/slow/shapes/idx_arr_null_still_checks_arraykey.php.expectf index 0d174617a9a..59867b032b9 100644 --- a/hphp/test/slow/shapes/idx_arr_null_still_checks_arraykey.php.expectf +++ b/hphp/test/slow/shapes/idx_arr_null_still_checks_arraykey.php.expectf @@ -1 +1 @@ -Fatal error: Argument 2 passed to HH\Shapes::idx() must be an instance of arraykey, null given in %s/test/slow/shapes/idx_arr_null_still_checks_arraykey.php on line 5 +Catchable fatal error: Argument 2 passed to HH\Shapes::idx() must be an instance of arraykey, null given in %s/test/slow/shapes/idx_arr_null_still_checks_arraykey.php on line 5 diff --git a/hphp/test/slow/shapes/idx_arr_type_error.php.expectf b/hphp/test/slow/shapes/idx_arr_type_error.php.expectf index f8458966d7e..f7ec638e74c 100644 --- a/hphp/test/slow/shapes/idx_arr_type_error.php.expectf +++ b/hphp/test/slow/shapes/idx_arr_type_error.php.expectf @@ -1,3 +1 @@ -Fatal error: Uncaught exception 'RuntimeException' with message 'idx() expects parameter 1 to be %rd?%rarray, integer given' in %s/test/slow/shapes/idx_arr_type_error.php:5 -Stack trace: -%A +Catchable fatal error: Argument 1 to HH\Shapes::idx() must be of type ?HH\darray, int given in %s/idx_arr_type_error.php on line 5 diff --git a/hphp/test/slow/shapes/idx_key_type_error.php.expectf b/hphp/test/slow/shapes/idx_key_type_error.php.expectf index 55956cec71e..fe5b675c710 100644 --- a/hphp/test/slow/shapes/idx_key_type_error.php.expectf +++ b/hphp/test/slow/shapes/idx_key_type_error.php.expectf @@ -1 +1 @@ -Fatal error: Argument 2 passed to HH\Shapes::idx() must be an instance of arraykey, null given in %s/test/slow/shapes/idx_key_type_error.php on line 5 +Catchable fatal error: Argument 2 passed to HH\Shapes::idx() must be an instance of arraykey, null given in %s/test/slow/shapes/idx_key_type_error.php on line 5 diff --git a/hphp/util/hphp-config.h.in b/hphp/util/hphp-config.h.in index eaa82f7a7a9..279b0c2e68d 100644 --- a/hphp/util/hphp-config.h.in +++ b/hphp/util/hphp-config.h.in @@ -90,7 +90,7 @@ ${HHVM_COMPILES_DEFINE_STRING} #endif #ifdef USE_CMAKE -# if ${HHVM_EXTENSION_COUNT} != 93 +# if ${HHVM_EXTENSION_COUNT} != 92 # error You need to update the config file for the new builtin extension, and add the define to the FB section # endif ${HHVM_EXTENSIONS_ENABLED_DEFINE_STRING} @@ -162,7 +162,6 @@ ${HHVM_EXTENSIONS_ENABLED_DEFINE_STRING} # define ENABLE_EXTENSION_REFLECTION 1 # define ENABLE_EXTENSION_SCRYPT 1 # define ENABLE_EXTENSION_SERVER 1 -# define ENABLE_EXTENSION_SHAPES 1 # define ENABLE_EXTENSION_SHMOP 1 # define ENABLE_EXTENSION_SIMPLEXML 1 # define ENABLE_EXTENSION_SNAPPY 1 -- 2.11.4.GIT