Skip to content

Commit

Permalink
Make inaccessible instead of null the default state of IC
Browse files Browse the repository at this point in the history
Summary:
After reviewing the strobelight data from Swayze, it is clear that the memoizations got a lot more expensive after the previous changes.

The main theory behind that is, now null and inaccessible contexts create 2 separate shards in the memo caches (before it was just null) depending on the calling context.

This diff makes Inaccessible as the default context. IC is not set null from anywhere. If it's needed, it can be implemented later via a www backdoor

Reviewed By: jano

Differential Revision: D57356181

fbshipit-source-id: c14f16adf5f5f116dbe8fc968d0ada8ff95652f2
  • Loading branch information
Max Trivedi authored and facebook-github-bot committed May 16, 2024
1 parent d3f9558 commit 3e2cf16
Show file tree
Hide file tree
Showing 26 changed files with 124 additions and 81 deletions.
3 changes: 3 additions & 0 deletions hphp/hack/src/hackc/emitter/emit_expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2970,6 +2970,9 @@ fn get_call_builtin_func_info(
"HH\\ImplicitContext\\_Private\\set_implicit_context_by_value" if e.systemlib() => {
Some((1, Instruct::Opcode(Opcode::SetImplicitContextByValue)))
}
"HH\\ImplicitContext\\_Private\\get_inaccessible_implicit_context" if e.systemlib() => {
Some((0, Instruct::Opcode(Opcode::GetInaccessibleImplicitContext)))
}
// TODO: enforce that this returns readonly
"HH\\global_readonly_get" => Some((1, Instruct::Opcode(Opcode::CGetG))),
"HH\\global_get" => Some((1, Instruct::Opcode(Opcode::CGetG))),
Expand Down
13 changes: 6 additions & 7 deletions hphp/runtime/base/execution-context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -653,8 +653,8 @@ void ExecutionContext::executeFunctions(ShutdownType type) {
Cfg::Server::PspCpuTimeoutSeconds
);

// Implicit context should not have leaked
assertx(!(*ImplicitContext::activeCtx));
// Since inaccessible is now used as the default context
assertx((*ImplicitContext::activeCtx) == (*ImplicitContext::inaccessibleCtx));

// We mustn't destroy any callbacks until we're done with all
// of them. So hold them in tmp.
Expand All @@ -669,8 +669,7 @@ void ExecutionContext::executeFunctions(ShutdownType type) {
[](TypedValue v) {
vm_call_user_func(VarNR{v}, init_null_variant,
RuntimeCoeffects::defaults());
// Implicit context should not have leaked between each call
assertx(!(*ImplicitContext::activeCtx));
assertx((*ImplicitContext::activeCtx) == (*ImplicitContext::inaccessibleCtx));
}
);
tmp.append(funcs);
Expand Down Expand Up @@ -1430,12 +1429,12 @@ void ExecutionContext::requestInit() {
SystemLib::mergePersistentUnits();
}

assertx(!ImplicitContext::activeCtx.isInit());
ImplicitContext::activeCtx.initWith(nullptr);

assertx(!ImplicitContext::inaccessibleCtx.isInit());
ImplicitContext::inaccessibleCtx.initWith(initInaccessibleConext().detach());

assertx(!ImplicitContext::activeCtx.isInit());
ImplicitContext::activeCtx.initWith(*ImplicitContext::inaccessibleCtx);

profileRequestStart();

HHProf::Request::StartProfiling();
Expand Down
2 changes: 1 addition & 1 deletion hphp/runtime/base/implicit-context.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ static Variant getBlameVectors();
struct Saver {
Saver() {
m_context = *ImplicitContext::activeCtx;
*ImplicitContext::activeCtx = nullptr;
*ImplicitContext::activeCtx = *ImplicitContext::inaccessibleCtx;
}
~Saver() {
*ImplicitContext::activeCtx = m_context;
Expand Down
8 changes: 6 additions & 2 deletions hphp/runtime/ext/hh/ext_implicit_context.php
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,9 @@ enum class MemoizeOption: string {
function backdoor<Tout>(
(function()[defaults]: Tout) $fn
)[/* 86backdoor */]: Tout {
$prev = \HH\ImplicitContext\_Private\set_implicit_context_by_value(null);
$prev = \HH\ImplicitContext\_Private\set_implicit_context_by_value(
\HH\ImplicitContext\_Private\get_inaccessible_implicit_context(),
);
try {
return $fn();
} finally {
Expand All @@ -202,7 +204,9 @@ function backdoor<Tout>(
async function backdoor_async<Tout>(
(function()[defaults]: Awaitable<Tout>) $fn
)[/* 86backdoor */]: Awaitable<Tout> {
$prev = \HH\ImplicitContext\_Private\set_implicit_context_by_value(null);
$prev = \HH\ImplicitContext\_Private\set_implicit_context_by_value(
\HH\ImplicitContext\_Private\get_inaccessible_implicit_context()
);
try {
$result = $fn();
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
<<__EntryPoint>>
function main_error() :mixed{
echo "Reading implicit context\n";
var_dump(IntContext::get());
try {
var_dump(IntContext::get());
} catch (InvalidOperationException $e){
echo $e->getMessage();
}
}

// FILE: main.php
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Sandbox: 0 (sandbox0) Version: 0
--------------------------------------------------------
Reading implicit context
NULL
Implicit context is set to inaccessible
--------------------------------------------------------
7 changes: 5 additions & 2 deletions hphp/test/slow/implicit-context/async-basic-4.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
<<__EntryPoint>>
async function main() :Awaitable<mixed>{
include 'async-implicit.inc';

var_dump(IntContext::getContext());
try {
var_dump(IntContext::getContext());
} catch (InvalidOperationException $e) {
echo $e->getMessage();
}
}
2 changes: 1 addition & 1 deletion hphp/test/slow/implicit-context/async-basic-4.php.expect
Original file line number Diff line number Diff line change
@@ -1 +1 @@
NULL
Implicit context is set to inaccessible
7 changes: 5 additions & 2 deletions hphp/test/slow/implicit-context/basic-4.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
<<__EntryPoint>>
function main() :mixed{
include 'implicit.inc';

var_dump(IntContext::getContext());
try {
var_dump(IntContext::getContext());
} catch (InvalidOperationException $e) {
echo $e->getMessage();
}
}
2 changes: 1 addition & 1 deletion hphp/test/slow/implicit-context/basic-4.php.expect
Original file line number Diff line number Diff line change
@@ -1 +1 @@
NULL
Implicit context is set to inaccessible
16 changes: 10 additions & 6 deletions hphp/test/slow/implicit-context/coeffect-2.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,35 @@ public static function set(int $context, (function (): int) $f)[zoned] :mixed{
return parent::runWith($context, $f);
}
public static function getContext()[zoned]: ?int {
return parent::get();
try {
return parent::get();
} catch (InvalidOperationException $e) {
echo $e->getMessage() . "\n";;
}
}
}

function g()[defaults] :mixed{
$context = IntContext::getContext() ?? 'null';
echo "in g context is $context \n";
echo "in g context is $context\n";
}

function h_exception()[defaults] :mixed{
$context = IntContext::getContext() ?? 'null';
echo "in h_exception context is $context \n";
echo "in h_exception context is $context\n";
echo "throwing exception from h_exception()\n";
throw new Exception();
}

function f()[zoned] :mixed{
$context = IntContext::getContext() ?? 'null';
echo "in f context is $context \n";
echo "in f context is $context\n";
HH\Coeffects\backdoor(g<>);
$context = IntContext::getContext() ?? 'null';
echo "back in f context is $context \n";
echo "back in f context is $context\n";
try { HH\Coeffects\backdoor(h_exception<>); } catch (Exception $_) {}
$context = IntContext::getContext() ?? 'null';
echo "back in f context is $context \n";
echo "back in f context is $context\n";
}

<<__EntryPoint>>
Expand Down
12 changes: 7 additions & 5 deletions hphp/test/slow/implicit-context/coeffect-2.php.expect
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
Setting context to 5
in f context is 5
in g context is null
back in f context is 5
in h_exception context is null
in f context is 5
Implicit context is set to inaccessible
in g context is null
back in f context is 5
Implicit context is set to inaccessible
in h_exception context is null
throwing exception from h_exception()
back in f context is 5
back in f context is 5
4 changes: 3 additions & 1 deletion hphp/test/slow/implicit-context/gc.php.expect
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
NULL
vec(1) {
string(14) "%Inaccessible%"
}
string(4) "done"
4 changes: 2 additions & 2 deletions hphp/test/slow/implicit-context/get_state_unsafe.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ function mici(): void {

<<__EntryPoint>>
function main(): void {
expect(HH\ImplicitContext\State::NULL);
expect(HH\ImplicitContext\State::INACCESSIBLE);
IntContext::set(
0,
() ==> expect(HH\ImplicitContext\State::VALUE),
);
expect(HH\ImplicitContext\State::NULL);
expect(HH\ImplicitContext\State::INACCESSIBLE);
mici();
}
2 changes: 1 addition & 1 deletion hphp/test/slow/implicit-context/keyed-1.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
function dump_ctx_data() :mixed{
try {
echo ClassContext::getContext()->name() . "\n";
} catch (Exception $e) {
} catch (InvalidOperationException $e) {
echo $e->getMessage() . "\n";
}
}
Expand Down
42 changes: 18 additions & 24 deletions hphp/test/slow/implicit-context/keyed-1.php.expectf
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
>>>>> from null
Before: Expected nonnull, got null
Expected nonnull, got null
Before: Implicit context is set to inaccessible
Implicit context is set to inaccessible
Hash:
NULL
vec(1) {
string(14) "%%Inaccessible%%"
}
memoNoArg passed
After: Expected nonnull, got null
After: Implicit context is set to inaccessible
------------------------
Before: Expected nonnull, got null
Expected nonnull, got null
Before: Implicit context is set to inaccessible
Implicit context is set to inaccessible
Hash:
NULL
vec(1) {
string(14) "%%Inaccessible%%"
}
memoWithArg passed
After: Expected nonnull, got null
After: Implicit context is set to inaccessible
------------------------
>>>>> from value
Before: C
Expand All @@ -36,20 +40,10 @@ memoWithArg passed
------------------------
>>>>> from inaccessible
Before: Implicit context is set to inaccessible
Implicit context is set to inaccessible
Hash:
vec(1) {
string(14) "%%Inaccessible%%"
}
memoNoArg passed
After: Implicit context is set to inaccessible
------------------------
Before: Implicit context is set to inaccessible
Implicit context is set to inaccessible
Hash:
vec(1) {
string(14) "%%Inaccessible%%"
}
memoWithArg passed
After: Implicit context is set to inaccessible
------------------------
Expand All @@ -63,14 +57,14 @@ memoWithArg passed
After: Implicit context is set to inaccessible
------------------------
>>>>> from soft run with
Before: Expected nonnull, got null
Before: Implicit context is set to inaccessible
memoNoArg passed
After: Expected nonnull, got null
After: Implicit context is set to inaccessible
------------------------
Before: Expected nonnull, got null
Before: Implicit context is set to inaccessible
memoWithArg passed
After: Expected nonnull, got null
After: Implicit context is set to inaccessible
------------------------
>>>>> final report
No arg ran: 3 times
With arg ran: 3 times
No arg ran: 2 times
With arg ran: 2 times
4 changes: 2 additions & 2 deletions hphp/test/slow/implicit-context/keyed-2.php.expect
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
No arg ran: 3 times
With arg ran: 3 times
No arg ran: 2 times
With arg ran: 2 times
18 changes: 14 additions & 4 deletions hphp/test/slow/implicit-context/memo-dynamic-1.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,27 @@ function f(bool $has_ctx) :mixed{
"memo_soft_ic_inaccessible",
];
foreach ($v as $f) {
if ($has_ctx) {
echo " Before: " . ClassContext::getContext()->name() . "\n";
try {
if ($has_ctx) {
echo " Before: " . ClassContext::getContext()->name() . "\n";
}
} catch (InvalidOperationException $e) {
echo $f . " failed with: " . $e->getMessage() . "\n";
}

try {
$f();
echo $f . " passed\n";
} catch (Exception $e) {
echo $f . " failed with: " . $e->getMessage() . "\n";
}
if ($has_ctx) {
echo " After: " . ClassContext::getContext()->name() . "\n";

try {
if ($has_ctx) {
echo " After: " . ClassContext::getContext()->name() . "\n";
}
} catch (InvalidOperationException $e) {
echo $f . " failed with: " . $e->getMessage() . "\n";
}
echo "------------------------\n";
}
Expand Down
2 changes: 2 additions & 0 deletions hphp/test/slow/implicit-context/memo-dynamic-1.php.expectf
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ memo_soft_ic_inaccessible passed
------------------------
memo_normal passed
------------------------
string(14) "%%Inaccessible%%"

memo_keyed passed
------------------------
memo_ic_inaccessible passed
Expand Down
5 changes: 5 additions & 0 deletions hphp/test/slow/implicit-context/no-ic-contexts.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,28 @@
<<__Memoize>>
function memo_with_leak_safe_zero_param()[leak_safe]: void {
$hash = HH\ImplicitContext\_Private\get_implicit_context_debug_info() ?? 'NULL';
$hash = HH\Lib\Str\join($hash, ", ");
echo "memo_with_leak_safe_zero_param hash: $hash\n";
}

<<__Memoize>>
function memo_with_leak_safe_single_param($a)[leak_safe]: void {
$hash = HH\ImplicitContext\_Private\get_implicit_context_debug_info() ?? 'NULL';
$hash = HH\Lib\Str\join($hash, ", ");
echo "memo_with_leak_safe_single_param hash: $hash\n";
}

<<__Memoize>>
function memo_with_pure_zero_param()[]: void {
$hash = HH\ImplicitContext\_Private\get_implicit_context_debug_info() ?? 'NULL';
$hash = HH\Lib\Str\join($hash, ", ");
echo "memo_with_pure_zero_param hash: $hash\n";
}

<<__Memoize>>
function memo_with_pure_single_param($a)[]: void {
$hash = HH\ImplicitContext\_Private\get_implicit_context_debug_info() ?? 'NULL';
$hash = HH\Lib\Str\join($hash, ", ");
echo "memo_with_pure_single_param hash: $hash\n";
}

Expand Down Expand Up @@ -52,6 +56,7 @@ function fn_with_leak_safe() [leak_safe]: void {
<<__Memoize>>
function no_param_with_leaksafe_only()[leak_safe]: void {
$hash = HH\ImplicitContext\_Private\get_implicit_context_debug_info() ?? 'NULL';
$hash = HH\Lib\Str\join($hash, ", ");
echo "no_param_with_leaksafe_only hash: $hash";
echo "\n";
}
Expand Down
10 changes: 5 additions & 5 deletions hphp/test/slow/implicit-context/no-ic-contexts.php.expect
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
defaults memo_no_param hash: %Inaccessible%
defaults memo_single_param hash: %Inaccessible%
memo_with_leak_safe_zero_param hash: NULL
memo_with_leak_safe_single_param hash: NULL
memo_with_pure_zero_param hash: NULL
memo_with_pure_single_param hash: NULL
no_param_with_leaksafe_only hash: NULL
memo_with_leak_safe_zero_param hash: %Inaccessible%
memo_with_leak_safe_single_param hash: %Inaccessible%
memo_with_pure_zero_param hash: %Inaccessible%
memo_with_pure_single_param hash: %Inaccessible%
no_param_with_leaksafe_only hash: %Inaccessible%
no_param_with_leaksafe_and_defaults hash: %Inaccessible%
no_param_with_leaksafe_local_only hash: %Inaccessible%
no_param_with_leaksafe_shallow_only hash: %Inaccessible%

0 comments on commit 3e2cf16

Please sign in to comment.