Skip to content

Conversation

dstogov
Copy link
Member

@dstogov dstogov commented Oct 3, 2024

No description provided.

@kocsismate
Copy link
Member

This PR brings around 0.88% perf improvement for Laravel. The rest of the tests are very similar as before.

@dstogov dstogov requested review from iluuu1994 and nielsdos October 3, 2024 21:08
return fbc;
}

static zend_execute_data* ZEND_FASTCALL zend_jit_push_this_metod_call_frame(zend_class_entry *scope, zend_function *fbc, uint32_t num_args)
Copy link
Member

Choose a reason for hiding this comment

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

Typo: metod -> method

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.


ZEND_ASSERT(Z_TYPE_P(zv) == IS_STRING);
class_name = Z_STR_P(zv);
ce = zend_lookup_class_ex(class_name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be beneficial to use the runtime cache to save some compilation time? In the tracing JIT the cache is probably primed. We could also use it as profiling data in some cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

This may be a good idea, but I would prefer to land this patch as is and than think about data-profiling improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Of course! This was just a thought I had while looking the change, but I agree it's not within the scope of this PR

if (op_array->fn_flags & ZEND_ACC_STATIC) {
scope_ref = ir_LOAD_A(jit_EX(This.value.ref));
} else {
scope_ref = ir_LOAD_A(ir_ADD_OFFSET(jit_EX(This.value.ref), offsetof(zend_object, ce)));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
scope_ref = ir_LOAD_A(ir_ADD_OFFSET(jit_EX(This.value.ref), offsetof(zend_object, ce)));
scope_ref = ir_LOAD_A(ir_ADD_OFFSET(ir_LOAD_A(jit_EX(This.value.ref)), offsetof(zend_object, ce)));

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! You are right.

}
}

if (opline->op1_type == IS_CONST) {
Copy link
Member

@nielsdos nielsdos Oct 4, 2024

Choose a reason for hiding this comment

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

This check with the "then" and "else" branch are almost identically the same to what happens in zend_jit_fetch_static_prop, except for the op number. Perhaps this can be factored out to a separate method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Done!

@dstogov dstogov merged commit 33b4bdc into php:master Oct 7, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants