Call __IsFoldable functions at jit time when possible. #7025

gdbentley · 2016-04-28T06:10:15Z

This change improves WordPress by 2.5% when run in non-repo mode. MediaWiki improves 0.5%.

FCallBuiltin invocations get optimized but not FCall invocations. Those can be handled in a follow-on.

ghost · 2016-04-28T06:23:17Z

This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D57369

hhvm-bot · 2016-08-29T20:12:28Z

@gdbentley updated the pull request - view changes - changes since last import

hhvm-bot · 2016-08-29T20:22:59Z

@gdbentley updated the pull request - view changes - changes since last import

aorenste · 2016-08-31T18:42:51Z

New test failures (4) that were not failing on base rev:

hphp/facebook/test/push-blocking:verify_quick_push_blocking - hphp/test/quick/dict/reflect.php: failed
hphp/facebook/test/push-blocking:verify_quick_push_blocking - hphp/test/quick/keyset/reflect.php: failed
hphp/facebook/test/push-blocking:verify_quick_push_blocking - hphp/test/quick/reflect.php: failed
hphp/facebook/test/push-blocking:verify_quick_push_blocking - hphp/test/quick/vec/reflect.php: failed

hhvm-bot · 2016-09-10T02:18:27Z

@gdbentley updated the pull request - view changes - changes since last import

aorenste · 2016-09-12T15:17:54Z

Latest version looks like it has some compilation errors:

hphp/runtime/vm/bytecode.cpp:1478:17: error: ‘HPHP::jit::Translator’ has not been declared
     !HPHP::jit::Translator::WriteLease().amOwner();
                 ^

hhvm-bot · 2016-09-14T03:30:21Z

@gdbentley updated the pull request - view changes - changes since last import

mxw

Looks good overall! Mostly nits, with one more major question for you.

I didn't audit the list of annotated functions in this pass. I will, but @Orvid, @paulbiss, @aorenste, can at least one of you take a pass also?

mxw · 2016-09-20T05:38:33Z

hphp/runtime/base/execution-context.h

+  ThrowAllErrorsSetter();
+  ~ThrowAllErrorsSetter();
+
+  private:


nit: Unindent.

mxw · 2016-09-20T05:38:42Z

hphp/runtime/base/request-injection-data-inl.h

@@ -33,6 +33,15 @@ inline bool RequestInjectionData::getJit() const {
  return m_jit;
 }

+inline bool RequestInjectionData::getJitFolding() const {
+  return m_jitFolding;
+  return true;


mxw · 2016-09-20T05:39:07Z

hphp/runtime/base/runtime-error.cpp

@@ -128,8 +128,9 @@ void raise_recoverable_error(const char *fmt, ...) {
 static int64_t g_notice_counter = 0;

 static bool notice_freq_check(ErrorMode mode) {
-  if (RuntimeOption::NoticeFrequency <= 0 ||
-      g_notice_counter++ % RuntimeOption::NoticeFrequency != 0) {
+  if (!g_context->getThrowAllErrors() &&


mxw · 2016-09-20T05:40:16Z

hphp/runtime/ext/std/ext_std_variable.cpp

+      auto size = str->size();
+      if (size >= RuntimeOption::MaxSerializedStringSize) {
+        throw Exception("Size of serialized string (%d) exceeds max", size);
+      }


Can you split this off into a separate PR?

Without this change, hphp/test/quick/string_replace_overflow.php will fail. See the review in phabricator D57369.

mxw · 2016-09-20T05:40:30Z

hphp/runtime/ext/std/ext_std_variable.cpp

@@ -247,11 +247,15 @@ String HHVM_FUNCTION(serialize, const Variant& value) {
    case KindOfPersistentString:
    case KindOfString: {
      StringData *str = value.getStringData();
+      auto size = str->size();


(For the PR in question below, auto const here.)

mxw · 2016-09-20T05:50:50Z

hphp/runtime/vm/jit/irgen-builtin.cpp

+    // ensure that vmpc is valid for later asserts within the invoke
+    vmpc() = vmfp()->m_func->getEntry();
+    SCOPE_EXIT{ vmpc() = savedPC; };
+#endif


Are the vmRegs always clean when we get here? If not, you need a VMRegAnchor; if so, it's probably not so bad to do this even in opt builds, since it's not very expensive.

Also, use if (debug) { ... } instead of the ifdefs.

I can't use if (debug) since that would create a new scope and the SCOPE_EXIT would execute at the wrong time.

mxw · 2016-09-20T05:54:36Z

hphp/runtime/vm/jit/irgen-builtin.cpp

+  auto const numNonVariadicArgs = (int32_t)func->numNonVariadicParams();
+  PackedArrayInit args(numArgs);
+  for (auto bcOff = BCSPRelOffset{(int32_t)numArgs - 1};
+       bcOff > BCSPRelOffset{(int32_t)(numArgs - numNonDefaultArgs - 1)};


Use static_cast.

mxw · 2016-09-20T05:54:56Z

hphp/runtime/vm/jit/irgen-builtin.cpp

+    SCOPE_EXIT{ vmpc() = savedPC; };
+#endif
+    SCOPE_EXIT{ RID().setJitFolding(false); };
+    RID().setJitFolding(true);


Swap these two lines (for consistency with the above).

mxw · 2016-09-20T05:55:54Z

hphp/runtime/vm/jit/irgen-builtin.cpp

+      return cns(env, makeStaticString(retVal.m_data.pstr));
+    } else if (isArrayLikeType(retVal.m_type)) {
+      return cns(env, make_array_like_tv(
+                     ArrayData::GetScalarArray(retVal.m_data.parr)));


nit: Something like

auto const arr = ArrayData::getScalarArray(retVal.m_data.parr); return cns(env, make_array_like_tv(arr));

mxw · 2016-09-20T06:01:16Z

hphp/runtime/ext/hash/ext_hash.php

@@ -33,7 +33,7 @@ function hash_algos(): array<string>;
 *
 * @param string $algo     - Name of selected hashing algorithm
 *                           (i.e. "md5", "sha256", "haval160,4", etc..)
- * @param string $filename - File who's contents are to be hashed.
+ * @param string $filename - File whose contents are to be hashed.


Thanks for cleaning these up while you're here :]

mxw · 2016-09-20T06:07:53Z

Running internal performance tests as well. Sorry that getting this diff in has taken so long—I just now saw that it's been open since April :\

simonwelsh · 2016-09-20T06:26:42Z

hphp/runtime/ext/std/ext_std_options.php

@@ -295,7 +295,7 @@ function php_sapi_name(): string;
 * displaying the OS PHP was built on. This will only happen if your uname()
 * library call either doesn't exist or doesn't work.
 */
-<<__Native>>
+<<__Native, __IsFoldable>>


I don't think this can be foldable. php_uname('n') can change are run time (if the hostname of the machine changes without restarting HHVM)

You're right. I forgot about the machine name; I was only thinking about the OS version.

hhvm-bot · 2016-09-21T19:58:54Z

@gdbentley updated the pull request - view changes - changes since last import

gdbentley · 2016-09-22T17:34:44Z

@mxw Completed rebase number 3.

hhvm-bot · 2016-09-22T17:35:16Z

@gdbentley updated the pull request - view changes - changes since last import

mxw · 2016-09-23T18:43:08Z

This seems to fail catastrophically on our internal integration tests. I'll look into why today or Monday and report back.

mxw · 2016-09-27T20:48:08Z

Okay, I'm pretty sure most of the failures were related to our test-infra. I believe those have been fixed, so I'm testing again.

markw65 · 2016-09-28T14:21:13Z

hphp/runtime/ext/fb/ext_fb.php

@@ -91,7 +91,7 @@ function fb_rename_function(string $orig_func_name,
 * @param mixed $input - What string to sanitize.
 * @return bool - Sanitized string.
 */
-<<__HipHopSpecific, __Native>>
+<<__HipHopSpecific, __Native, __IsFoldable>>
 function fb_utf8ize(mixed &$input): bool;


I don't see where you're dealing with reference parameters... won't this just "optimize" it to the bool result, while failing to sanitize $input?

It's not possible to pass const input to this function. I've removed the __IsFoldable.

markw65 · 2016-09-28T14:24:36Z

hphp/runtime/ext/string/ext_string.php

@@ -568,7 +569,7 @@ function str_pad(string $input,
 * @return string - Returns the repeated string.
 *
 */
-<<__Native>>
+<<__IsFoldable, __Native>>
 function str_repeat(string $input, int $multiplier): string;


This seems risky - what about str_repeat('x', 2000000000) ? The resulting static string is going to live forever. Maybe there should be a memory limit on the size of the result?

That seems like an application problem to me. Anyway, if your PHP has code like that, wouldn't you prefer to have one copy of the long string rather than allocate it over and over again for each request?

markw65 · 2016-09-28T14:25:50Z

hphp/runtime/ext/string/ext_string.php

@@ -1676,7 +1677,7 @@ function levenshtein(string $str1,
 * @return int - Returns the number of matching chars in both strings.
 *
 */
-<<__Native>>
+<<__IsFoldable, __Native>>
 function similar_text(string $first,
                      string $second,
                      mixed &$percent = null): int;


more reference parameters... I guess this would be ok as long as you check that no parameter is passed for this arg...

If a parameter is passed, it has to be a variable (so that a reference can be taken). If it's a variable, it can't be constant (null or a literal). I've just tested this function on my branch and a two parameter invocation with string literals gets folded while a three parameter invocation does not.

markw65 · 2016-09-28T14:31:30Z

hphp/runtime/vm/jit/irgen-builtin.cpp

+    }
+  }
+  if (numArgs != func->numNonVariadicParams()) {
+    if (!topType(env).hasConstVal()) return nullptr;


You should check hasConstVal(TArr), or (since I think the emitter should guarantee that) add an assert that constAsCell(topC(env)) is an array.

Added assert.

markw65 · 2016-09-28T14:33:48Z

hphp/runtime/vm/jit/irgen-builtin.cpp

+    g_context->invokeFunc(&retVal, func, args.toArray(), nullptr, nullptr,
+                          nullptr, nullptr, ExecutionContext::InvokeNormal,
+                          !func->unit()->useStrictTypes());
+


When we start jitting in separate threads (real soon now), this will almost certainly break. It might already break due to some of the guards that have recently been added to try to enforce that...

What specifically is the concern? That the thread doing the jitting will not have a valid g_context? Should I switch back to the (deprecated) invoke() code?

markw65

Maybe we can never call builtins with reference parameters, because the params will never be constants - but I think we should be explicit about it; maybe even check in the emitter that foldable functions don't have non-default reference params.

I'm also wondering worried about separate jit threads...

hhvm-bot · 2016-09-28T19:28:37Z

@gdbentley updated the pull request - view changes - changes since last import

mxw · 2016-10-06T16:19:01Z

@gdbentley—Okay, sorry for the delay; we went through several iterations of the protection mechanism before it was fully correct. The issue is resolved by a one-liner (VMProtect::Pause foo; above your VMRegAnchor), which I've already added to the internal copy of the diff. Tests seem to be passing just fine, so I'm going to rebase and start them again just to be sure. Unless there's something you want to change, no action is needed on your part—I'll land the diff with that update via our usual internal mechanism.

gdbentley · 2016-10-06T17:53:51Z

@mxw Sounds good. Thanks.

ghost added the CLA Signed label Apr 28, 2016

gdbentley force-pushed the fold_functions branch from 2288072 to b4b910d Compare August 27, 2016 00:40

ghost added the CLA Signed label Aug 27, 2016

ghost added the CLA Signed label Aug 29, 2016

aorenste added the GH Review: needs-revision label Aug 31, 2016

facebook-github-bot added the CLA Signed label Aug 31, 2016

mxw self-assigned this Sep 9, 2016

facebook-github-bot added the CLA Signed label Sep 9, 2016

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Sep 10, 2016

ghost added the CLA Signed label Sep 10, 2016

aorenste added GH Review: needs-revision and removed GH Review: review-needed labels Sep 12, 2016

ghost added the CLA Signed label Sep 12, 2016

gdbentley force-pushed the fold_functions branch from c45f894 to 1d4be4a Compare September 14, 2016 03:30

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Sep 14, 2016

ghost added the CLA Signed label Sep 14, 2016

mxw suggested changes Sep 20, 2016

View reviewed changes

simonwelsh reviewed Sep 20, 2016

View reviewed changes

ghost added the CLA Signed label Sep 21, 2016

gdbentley added 9 commits September 22, 2016 10:31

Revert accidental formatting change.

286f809

tab -> spaces

09d818c

Changes based on review.

db61dad

More review changes.

bc7f92a

Adjusted to deal with Hack arrays.

8afaac5

Fixed failed assertions.

3048d98

Rebased again and dealt with new breakages.

9188eea

More review changes.

e9ed690

Rebase number 3.

f74174e

gdbentley force-pushed the fold_functions branch from f198660 to f74174e Compare September 22, 2016 17:34

ghost added the CLA Signed label Sep 22, 2016

mxw approved these changes Sep 27, 2016

View reviewed changes

markw65 reviewed Sep 28, 2016

View reviewed changes

markw65 suggested changes Sep 28, 2016

View reviewed changes

Review changes.

04fbb1a

facebook-github-bot added the CLA Signed label Sep 28, 2016

facebook-github-bot added the CLA Signed label Oct 6, 2016

hhvm-bot closed this in 6af75f1 Oct 19, 2016

Call __IsFoldable functions at jit time when possible. #7025

Call __IsFoldable functions at jit time when possible. #7025

Conversation

gdbentley commented Apr 28, 2016

ghost commented Apr 28, 2016

hhvm-bot commented Aug 29, 2016

hhvm-bot commented Aug 29, 2016

aorenste commented Aug 31, 2016

hhvm-bot commented Sep 10, 2016

aorenste commented Sep 12, 2016

hhvm-bot commented Sep 14, 2016

mxw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxw Sep 20, 2016 • edited

Choose a reason for hiding this comment

gdbentley Sep 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxw commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhvm-bot commented Sep 21, 2016

gdbentley commented Sep 22, 2016

hhvm-bot commented Sep 22, 2016

mxw commented Sep 23, 2016

mxw commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markw65 Sep 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markw65 left a comment

Choose a reason for hiding this comment

hhvm-bot commented Sep 28, 2016

mxw commented Oct 6, 2016

gdbentley commented Oct 6, 2016

mxw Sep 20, 2016 •

edited

gdbentley Sep 21, 2016 •

edited

markw65 Sep 28, 2016 •

edited