Improve performance of ZEND_IS_IDENTICAL #4900

TysonAndre · 2019-11-10T22:55:01Z

And for ZEND_IS_NOT_IDENTICAL.

Take advantage of branch prediction in cases that are false/true
Avoid function call and duplicate fetches of Z_TYPE_P in common use cases
Avoid checking for EG(exception) when it's impossible.

I think that the zval would always be IS_NULL or lower for an exception,
but may be wrong.

Aside 1: I see that ZEND_IS_EQUAL does have the SMART_BRANCH in the
SPEC, generating optimized implementations based on what type of smart
branch is used. Is excluding SMART_BRANCH from ZEND_IS_IDENTICAL deliberate,
and if so, what's the reasoning?

Aside 2: The function pointer for ZEND_IS_EQUAL_LONG can probably be used
directly for ZEND_IS_IDENTICAL_LONG. I'm not sure how to generate that.
(and DOUBLE)

The amount of time for nestedloop_ni(20) decreased from 0.726s to
0.593s due to this change.
This is still worse than != (0.434) and < (0.375), which
have implementations that take advantage of branch prediction for
IS_LONG (i.e. EXPECTED blocks)

Still 0.593 after the latest patch

function nestedloop_ni($n) {
  $x = 0;
  for ($a=0; $a!==$n; $a++)
    for ($b=0; $b!==$n; $b++)
      for ($c=0; $c!==$n; $c++)
        for ($d=0; $d!==$n; $d++)
          for ($e=0; $e!==$n; $e++)
            for ($f=0; $f!==$n; $f++)
             $x++;
  print "$x\n";
}

TysonAndre · 2019-11-10T23:39:04Z

The build failure seems unrelated - tests/basic/timeout_variation_4.phpt doesn't use !== or ==, and passes locally - the appveyor error is a test of windows filesystems.

cc @nikic and @dstogov , who were the last committers to zend_vm_def.h

dstogov · 2019-11-11T07:39:41Z

Zend/zend_vm_def.h

 	FREE_OP1();
 	FREE_OP2();
-	ZEND_VM_SMART_BRANCH(result, 1);
+	ZEND_VM_SMART_BRANCH(result, 0);


It looks like, you messed the meaning of the second argument of ZEND_VM_SMART_BRANCH().
It should be 1 here (because FREE_OP on IS_OBJECT may call destructor that throws exception) and 0 in other places.

TysonAndre · 2019-11-11T15:12:23Z

Zend/zend_vm_def.h

+		/* This has to check for undefined variable errors when IS_NULL is possible. */
+		FREE_OP1();
+		FREE_OP2();
+		ZEND_VM_SMART_BRANCH(1, 1);


I guess this can probably be ((OP1_TYPE & IS_CV) || (OP2_TYPE & IS_CV)), if only CVs can be undefined variables. http://nikic.github.io/2017/04/14/PHP-7-Virtual-machine.html#variable-types

nikic · 2019-11-12T14:13:49Z

Zend/zend_vm_def.h

@@ -451,7 +451,50 @@ ZEND_VM_COLD_CONSTCONST_HANDLER(16, ZEND_IS_IDENTICAL, CONST|TMP|VAR|CV, CONST|T
 	SAVE_OPLINE();
 	op1 = GET_OP1_ZVAL_PTR_DEREF(BP_VAR_R);
 	op2 = GET_OP2_ZVAL_PTR_DEREF(BP_VAR_R);
-	result = fast_is_identical_function(op1, op2);
+	if (Z_TYPE_P(op1) != Z_TYPE_P(op2)) {


I'd suggest to fetch op1 as UNDEF and handle the REF/UNDEF cases explicitly, so we get a fast path if they do not apply.

I was actually working on that locally - I didn't see much of a performance boost locally (I assume it's a compare and conditional jump, for 2 instructions)

Maybe moving an is_identical_function implementation into the

I was also thinking of skipping the IS_UNDEF check for const/tmp/var, and IS_INDIRECT for everything except var

https://github.com/TysonAndre/php-src/pull/new/ZEND_IS_IDENTICAL-wip-broken - will fix this later today or tomorrow.

I forgot to check for IS_INDIRECT in the case for if (Z_TYPE_P(op1) != Z_TYPE_P(op2)) {

The issue I was having was caused by opcache being enabled in the php.ini that would have been used by the installation. I think IS_INDIRECT still needed to be handled

And for ZEND_IS_NOT_IDENTICAL. - Take advantage of branch prediction in cases that are false/true - Avoid function call and duplicate fetches of Z_TYPE_P in common use cases - Avoid checking for EG(exception) when it's impossible. I think that the zval would always be IS_NULL or lower for an exception, but may be wrong. Aside 1: I see that ZEND_IS_EQUAL does have the `SMART_BRANCH` in the `SPEC`, generating optimized implementations based on what type of smart branch is used. Is excluding it from ZEND_IS_IDENTICAL deliberate, and if so, what's the reasoning? (e.g. is the use in common applications low?) - The SMART_BRANCH specialization was introduced in 27e01cd Aside 2: The function pointer for ZEND_IS_EQUAL_LONG can probably be used directly for ZEND_IS_IDENTICAL_LONG. I'm not sure how to generate that. (and DOUBLE) The amount of time for nestedloop_ni(20) decreased from 0.726s to 0.593s due to this change. This is still worse than `!=` (0.434) and `<` (0.375), which have implementations that take advantage of branch prediction for IS_LONG ```php function nestedloop_ni($n) { $x = 0; for ($a=0; $a!==$n; $a++) for ($b=0; $b!==$n; $b++) for ($c=0; $c!==$n; $c++) for ($d=0; $d!==$n; $d++) for ($e=0; $e!==$n; $e++) for ($f=0; $f!==$n; $f++) $x++; print "$x\n"; } ```

Locally, I didn't see that much of a performance boost in absolute terms compared to inlining zend_is_identical (compare+unlikely jump is probably 2 instructions)

Girgias · 2019-11-13T10:49:55Z

macOS fails to build

Girgias · 2019-11-13T20:49:46Z

Still 2 macOS build error:

[command]/bin/bash --noprofile --norc /Users/runner/runners/2.160.0/work/_temp/60d85ccc-36a6-49cc-8573-c3710f411662.sh
In file included from /Users/runner/runners/2.160.0/work/1/s/Zend/zend_execute.c:4468:
Zend/zend_vm_execute.h:46519:24: error: use of logical '||' with constant operand [-Werror,-Wconstant-logical-operand]
                if (((IS_CV & IS_CV) || (IS_CV & IS_CV)) && UNEXPECTED(Z_TYPE_P(op1) == IS_UNDEF)) {
                                     ^  ~~~~~~~~~~~~~~~
Zend/zend_vm_execute.h:46519:24: note: use '|' for a bitwise operation
                if (((IS_CV & IS_CV) || (IS_CV & IS_CV)) && UNEXPECTED(Z_TYPE_P(op1) == IS_UNDEF)) {
                                     ^~
                                     |
Zend/zend_vm_execute.h:46620:24: error: use of logical '||' with constant operand [-Werror,-Wconstant-logical-operand]
                if (((IS_CV & IS_CV) || (IS_CV & IS_CV)) && UNEXPECTED(Z_TYPE_P(op1) == IS_UNDEF)) {
                                     ^  ~~~~~~~~~~~~~~~
Zend/zend_vm_execute.h:46620:24: note: use '|' for a bitwise operation
                if (((IS_CV & IS_CV) || (IS_CV & IS_CV)) && UNEXPECTED(Z_TYPE_P(op1) == IS_UNDEF)) {
                                     ^~
                                     |
2 errors generated.
make: *** [Zend/zend_execute.lo] Error 1

dstogov · 2019-11-15T15:28:19Z

The PR makes some improvement using aggressive code inlining.
In my opinion, this doesn't cost 10K code increase.

The typical use of ===/!== is a comparison with a constant.
I think, 3 type specialised opcodes (ZEND_VM_[HOT]_TYPE_SPEC_HANDLER) for comparison with int/double/string constant will make much better results with less overhead.

@TysonAndre you may also think about general specialization on types of constants. This is more serious and questionable task. It may eliminate a lot of useless run-time cheks in VM, but this also may significantly increase the code size. So it's hard to say, if this is going to be accepted before we see a PoC.

TysonAndre · 2019-11-19T14:46:39Z

The typical use of ===/!== is a comparison with a constant.
I think, 3 type specialised opcodes (ZEND_VM_[HOT]_TYPE_SPEC_HANDLER) for comparison with int/double/string constant will make much better results with less overhead.

Is that for

op2_type == MAY_BE_STRING (I assume), or
op1_type == MAY_BE_STRING && op2_type == MAY_BE_STRING

(there are now reused handlers for ZEND_IS_IDENTICAL handling both sides being longs and both sides being doubles)

It'd make sense that comparison with scalars would be more common in a typical application

dstogov · 2019-11-19T21:22:21Z

I meant:

ZEND_VM_HOT_TYPE_SPEC_HANDLER(ZEND_IS_IDENTICAL, (op->op2_type == IS_CONST && RT_CONSTANT(op, op->op2) == IS_LONG), ZEND_IS_IDENTICAL_LONG, TMPVARCV, CONST)

ZEND_VM_HOT_TYPE_SPEC_HANDLER(ZEND_IS_IDENTICAL, (op->op2_type == IS_CONST && RT_CONSTANT(op, op->op2) == IS_DOUBLE), ZEND_IS_IDENTICAL_DOUBLE, TMPVARCV, CONST)

ZEND_VM_HOT_TYPE_SPEC_HANDLER(ZEND_IS_IDENTICAL, (op->op2_type == IS_CONST && RT_CONSTANT(op, op->op2) == IS_STRING), ZEND_IS_IDENTICAL_STRING, TMPVARCV, CONST)
...

This has a large performance benefit for the below snippet (0.64s vs 0.94s), and a small benefit for `$alwaysStr == 'notnumeric'`/`$s = 'other'` (for 1000 runs of test('test')) - If the literal is non-numeric and the variable is a string, `==` behaves identically to `===`, so reuse the opcode. This is similar to what was suggested in php#4900 - Avoiding types with __destruct or notices simplifies the specialized implementation and avoids expensive exception handling. - TMPVARCV doesn't support FREE_OP1() - use TMPVAR|CV instead. ``` function test(string $s) : int { $total = 0; for ($i = 0; $i < 100000; $i++) { if ($s === "first") { $total++; $s = null; } } return $total; } ```

TysonAndre · 2019-12-07T20:38:20Z

Specialized type handlers seem to be a better approach - my subsequent PRs have better performance gains.

This has a large performance benefit for the below snippet (0.64s vs 0.94s), and a small benefit for `$alwaysStr == 'notnumeric'`/`$s = 'other'` (for 1000 runs of test('test')) - If the literal is non-numeric and the variable is a string, `==` behaves identically to `===`, so reuse the opcode. This is similar to what was suggested in php#4900 - Avoiding types with __destruct or notices simplifies the specialized implementation and avoids expensive exception handling. - TMPVARCV doesn't support FREE_OP1() - use TMPVAR|CV instead. ``` function test(string $s) : int { $total = 0; for ($i = 0; $i < 100000; $i++) { if ($s === "first") { $total++; $s = null; } } return $total; } ```

dstogov reviewed Nov 11, 2019

View reviewed changes

carusogabriel added the Feature label Nov 11, 2019

TysonAndre commented Nov 11, 2019

View reviewed changes

nikic reviewed Nov 12, 2019

View reviewed changes

TysonAndre added 3 commits November 12, 2019 09:51

Address review feedback

72ec3a6

Optimize for not being indirect/undef/references

2f96eba

Locally, I didn't see that much of a performance boost in absolute terms compared to inlining zend_is_identical (compare+unlikely jump is probably 2 instructions)

TysonAndre force-pushed the ZEND_IS_IDENTICAL-optimize branch from 7482b71 to 2f96eba Compare November 13, 2019 03:58

Fix MacOS build error

aec0a54

Fix undefined zval check

18a672b

TysonAndre force-pushed the ZEND_IS_IDENTICAL-optimize branch from c47e9b3 to 18a672b Compare November 13, 2019 22:14

TysonAndre mentioned this pull request Dec 6, 2019

Add specialized handler for checking if identical to string #4972

Open

TysonAndre closed this Dec 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of ZEND_IS_IDENTICAL #4900

Improve performance of ZEND_IS_IDENTICAL #4900

TysonAndre commented Nov 10, 2019 •

edited

TysonAndre commented Nov 10, 2019

dstogov Nov 11, 2019

TysonAndre Nov 11, 2019

nikic Nov 12, 2019

TysonAndre Nov 12, 2019

TysonAndre Nov 12, 2019 •

edited

TysonAndre Nov 13, 2019

Girgias commented Nov 13, 2019

Girgias commented Nov 13, 2019

dstogov commented Nov 15, 2019

TysonAndre commented Nov 19, 2019

dstogov commented Nov 19, 2019

TysonAndre commented Dec 7, 2019

Improve performance of ZEND_IS_IDENTICAL #4900

Improve performance of ZEND_IS_IDENTICAL #4900

Conversation

TysonAndre commented Nov 10, 2019 • edited

TysonAndre commented Nov 10, 2019

dstogov Nov 11, 2019

Choose a reason for hiding this comment

TysonAndre Nov 11, 2019

Choose a reason for hiding this comment

nikic Nov 12, 2019

Choose a reason for hiding this comment

TysonAndre Nov 12, 2019

Choose a reason for hiding this comment

TysonAndre Nov 12, 2019 • edited

Choose a reason for hiding this comment

TysonAndre Nov 13, 2019

Choose a reason for hiding this comment

Girgias commented Nov 13, 2019

Girgias commented Nov 13, 2019

dstogov commented Nov 15, 2019

TysonAndre commented Nov 19, 2019

dstogov commented Nov 19, 2019

TysonAndre commented Dec 7, 2019

TysonAndre commented Nov 10, 2019 •

edited

TysonAndre Nov 12, 2019 •

edited