Skip to content

Avoid JIT warning with opcache.jit_buffer_size=0 #12460

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.

Sign up for GitHub

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
Closed

Avoid JIT warning with opcache.jit_buffer_size=0 #12460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ext/opcache/ZendAccelerator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3270,7 +3270,11 @@ static zend_result accel_post_startup(void)
|| zend_jit_startup(ZSMMG(reserved), jit_size, reattached) != SUCCESS) {
JIT_G(enabled) = 0;
JIT_G(on) = 0;
zend_accel_error(ACCEL_LOG_WARNING, "Could not enable JIT!");
/* The JIT is implicitly disabled with opcache.jit_buffer_size=0, so we don't want to
* emit a warning here. */
if (JIT_G(buffer_size) != 0) {
Copy link
Contributor

@danog danog Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To be entirely honest, I'm not a huge fan of how JIT is disabled by default using the opcache.jit_buffer_size=0 default, instead of opcache.jit=0.

Do you think it would be OK to change the defaults of opcache.jit to 0, and opcache.jit_buffer_size to a sane default like 16m at least on master (I'd really love to make the same change on PHP-8.1 though...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that the INI values are sub-optimal. However, changing them in 8.1 would mean anybody who enabled JIT using jit_buffer_size without also setting jit would implicitly disable it. This could be changed for master, but should be discussed on the ML as it is a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is an RFC required, or just a simple email to discuss it OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

@danog Depends on whether there are any concerns. When there are concerns it's better to be safe and create a short RFC. Simple RFCs don't need to be long.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iluuu1994 Sent a discussion email + a karma request :)

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, what about instead of completely removing the error, it simply indicated that JIT wasn't enabled because the buffer size is 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the default configuration should emit a warning. It doesn't really, because log_verbosity_level needs to be increased. That's some INI weirdness by itself.

zend_accel_error(ACCEL_LOG_WARNING, "Could not enable JIT!");
}
}
}
#endif
Expand Down
16 changes: 16 additions & 0 deletions ext/opcache/tests/jit_warning_with_zero_buffer.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
--TEST--
JIT should not emit warning with opcache.jit_buffer_size=0
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=tracing
opcache.jit_buffer_size=0
opcache.log_verbosity_level=2
--EXTENSIONS--
opcache
--FILE--
<?php
var_dump(opcache_get_status()['jit']['enabled'] ?? false);
?>
--EXPECT--
bool(false)