Skip to content

Optimize ZEND_COUNT opcodes on arrays in the jit #5584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented May 16, 2020

Avoid the overhead of a call and checking types
when the argument is definitely an array.
Avoid the overhead of gc when __destruct won't get called.

This seemed cheap enough to check for in the jit.

EDIT: Because of https://wiki.php.net/rfc/restrict_globals_usage
we can be sure in the ZEND_COUNT handler that the array count does not have to
be recomputed in php 8.1.

The below example took 0.854 seconds before the optimization,
and 0.564 seconds after the optimization, giving the same result

<?php
/** @jit */
function bench_count(int $n): int {
    $total = 0;
    $arr = [];
    for ($i = 0; $i < $n; $i++) {
        $arr[] = $i;
        $total += count($arr);
    }
    return $total;
}

function main() {
    $n = 1000;
    $iterations = 50000;
    $start = microtime(true);
    $result = 0;
    for ($i = 0; $i < $iterations; $i++) {
        $result += bench_count($n);
    }
    $elapsed = microtime(true) - $start;

    printf("Total for n=%d, iterations=%d = %d, elapsed=%.3f\n", $n, $iterations, $result, $elapsed);
}
main();

Before

mov $0x7feb8cf8a858, %r15
mov $ZEND_COUNT_SPEC_CV_UNUSED_HANDLER, %rax
call *%rax

After (much faster without a function call)

mov 0x70(%r14), %rdi - Copy the count from the `zend_array*` pointer
mov %rdi, (%rax)     - Store the count in the destination's value
mov $0x4, 0x8(%rax)  - Store IS_LONG(4) in the destination's type

@TysonAndre TysonAndre force-pushed the opcache-jit-count branch 2 times, most recently from 4f45167 to c730720 Compare May 17, 2020 17:14
@TysonAndre TysonAndre force-pushed the opcache-jit-count branch 2 times, most recently from 9b6d568 to 79cd6aa Compare February 6, 2021 16:24
@TysonAndre
Copy link
Contributor Author

TysonAndre commented Feb 6, 2021

I updated this now that https://wiki.php.net/rfc/restrict_globals_usage has passed (related to 2279388 )

@TysonAndre TysonAndre requested a review from nikic February 6, 2021 16:35
if (!zend_jit_count(&dasm_state, opline, op_array, op1_info)) {
goto jit_failure;
}
goto done;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to add it to zend_jit_trace.c for it to be meaningfully useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in zend_jit_trace.c based on the handler for ZEND_STRLEN

// Anything containing objects or resources might throw when freed due to the destructors.
zend_bool gc = (op1_info & (MAY_BE_ARRAY_OF_OBJECT|MAY_BE_ARRAY_OF_ARRAY|MAY_BE_ARRAY_OF_RESOURCE|MAY_BE_ARRAY_OF_REF)) != 0;
| ZVAL_PTR_DTOR op1_addr, op1_info, gc, 0, opline
if (gc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the common practice here is to determine this using zend_may_throw (passed in).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to re-add this, I was running into issues because nNumOfElements was 32-bit when working on the tracing jit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, could use another look

| mov r0, [FCARG1a + offsetof(HashTable, nNumOfElements)]
| SET_ZVAL_LVAL res_addr, r0
| SET_ZVAL_TYPE_INFO res_addr, IS_LONG
if (opline->op1_type & (IS_VAR|IS_TMP_VAR)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use FREE_OP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@TysonAndre
Copy link
Contributor Author

@nikic @dstogov - This is ready for another look

Avoid the overhead of a call and checking types
when the argument is definitely an array.
Avoid the overhead of gc when `__destruct` won't get called.

This seemed cheap enough to check for in the jit.

Because of https://wiki.php.net/rfc/restrict_globals_usage
we can be sure in the ZEND_COUNT handler that the array count does not have to
be recomputed in php 8.1.

The below example took 0.854 seconds before the optimization,
and 0.564 seconds after the optimization, giving the same result

```php
<?php
/** @jit */
function bench_count(int $n): int {
    $total = 0;
    $arr = [];
    for ($i = 0; $i < $n; $i++) {
        $arr[] = $i;
        $total += count($arr);
    }
    return $total;
}

function main() {
    $n = 1000;
    $iterations = 50000;
    $start = microtime(true);
    $result = 0;
    for ($i = 0; $i < $iterations; $i++) {
        $result += bench_count($n);
    }
    $elapsed = microtime(true) - $start;

    printf("Total for n=%d, iterations=%d = %d, elapsed=%.3f\n", $n, $iterations, $result, $elapsed);
}
main();
```

Before

```asm
mov $0x7feb8cf8a858, %r15
mov $ZEND_COUNT_SPEC_CV_UNUSED_HANDLER, %rax
call *%rax
```

After

```asm
mov 0x70(%r14), %rdi - Copy the count from the `zend_array*` pointer
mov %rdi, (%rax)     - Store the count in the destination's value
mov $0x4, 0x8(%rax)  - Store IS_LONG(4) in the destination's type
```

And add tracing jit support
@dstogov
Copy link
Member

dstogov commented Feb 9, 2021

Everything should be fine. Merge this.

@php-pulls php-pulls closed this in d951034 Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants