Add API to exempt function from being traced in JIT #15559

bwoebi · 2024-08-23T18:39:02Z

Sometimes extensions need to do hideous stuff which is incompatible with JIT.
As such, I propose a simple API function to mark an op_array as blacklisted.

In the past we've been maintaining our own (https://github.com/DataDog/dd-trace-php/blob/master/zend_abstract_interface/jit_utils/jit_blacklist.c) solution to work around it, with a lot of dlsym and relying on JIT internals.

It would be great if that could be part of API of JIT itself.

dstogov

This is not a complete solution.

This will disable JIT only for traces started at function entry, but not for inner loops or the function inlining. This also won't disable the function JIT.

The function zend_jit_blacklist_function() exported from DLL, won't work on Windows.

Anyway, the idea makes sense. I would think about a special no-jit attribute and an ability to add this attribute at run-time. Then we may avoid function JIT compilation and set up of tracing JIT counters. Tracer also should be adjusted to stop the traces at enter/exit to no-jit functions.

bwoebi · 2024-08-26T18:37:32Z

@dstogov Thanks for pointing this out.

I was under the assumption that if you function inline, you'd include the start of the function, which ... contains that blacklisting point. But I admit I have little idea of the tracing JIT internals.
They honestly scare me :-) JIT itself is fine, but tracing JIT is scary.

I do just not know how to implement that properly then. Is this something you or maybe @iluuu1994 could actually help me out with?

I also do like the idea of special no-jit attributes, as it turns out that the JIT still has some stability issues despite being there for 4 years now, just to disable "known broken functions" as the user.

dstogov · 2024-08-28T06:55:15Z

I was under the assumption that if you function inline, you'd include the start of the function, which ... contains that blacklisting point. But I admit I have little idea of the tracing JIT internals. They honestly scare me :-) JIT itself is fine, but tracing JIT is scary.

There were a good paper about tracing jit - https://www.semanticscholar.org/paper/Trace-based-just-in-time-compilation-for-lazy-Schilling/8b47d69f96cbc08da737d1f78f3ffd4376f8e135
Unfortunately I don't see a public link now.

I do just not know how to implement that properly then. Is this something you or maybe @iluuu1994 could actually help me out with?

This should be simple. Tracer should stop the recording in case of entering/exiting to disabled function.
@iluuu1994 just made similar thing for property hooks.

iluuu1994 · 2024-08-28T09:58:50Z

For context: 606eb84

bwoebi · 2024-09-10T11:09:20Z

@dstogov Is it possible for you to have a look at this?

dstogov

See comments.

Note, this will prevent JIT-ing of the function, but not the execution of the previously JIT-ed traces that are started in different functions and inline (or return to) the blacklisted one.

dstogov · 2024-09-11T06:19:39Z

ext/opcache/jit/zend_jit.h

-void zend_jit_status(zval *ret);
+ZEND_EXT_API void zend_jit_status(zval *ret);
+ZEND_EXT_API void zend_jit_blacklist_function(zend_op_array *op_array);


Make a normal API not a yet another hack for your observer.

I don't understand what you mean? What would "a normal API" look like?

Make it work through use-level opcache_jit_blacklist() function (similar opcache_invalidate) or use "no-jit" PHP attribute, etc

Describe the API (behavior should not be driven by implementation details, it should satisfy the requirements).

dstogov · 2024-09-11T06:20:25Z

ext/opcache/jit/zend_jit_ir.c

+
+			if (trace) {
+				int32_t exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_TO_VM);
+
+				exit_addr = zend_jit_trace_get_exit_addr(exit_point);
+				if (!exit_addr) {
+					return 0;
+				}
+			} else {
+				exit_addr = NULL;
+			}
+
+			if (!zend_jit_check_timeout(jit, NULL /* we're inside the called function */, exit_addr)) {
+				return 0;
+			}
+


How this is related to the API?

Note, this will prevent JIT-ing of the function, but not the execution of the previously JIT-ed traces that are started in different functions and inline (or return to) the blacklisted one.

Yes, that's how this is related. It's an escape hatch how I am manually able to force-skip manually disabled functions by returning EG(vm_interrupt) in an observer. I have no better idea on how to achieve that short of keeping a mapping on what functions are inlined in what trace and using that to clear existing traces or such...

So you insert additional interrupt check just to check if the function was blacklisted at run-time and invalidate the current trace. How do you do this?

why can't you do this "black work" in jit_observer_fcall_begin()?

you still miss the case when trace ir returned to blacklisted function.

jit_observer_fcall_begin is also use by ICALLs, but for ICALL this is in the middle of the opcode. There's no way to safely resume there - the de-optimization has to point either at the ICALL opcode or the one after. In the former case it'll run observers twice (that's bad) or it'll bypass the internal handle call completely and skip the end handlers (that's bad too).

you still miss the case when trace ir returned to blacklisted function.

Yes. I'm not sure whether that's a problem though. But I also have no idea on how to avoid that.

I think, the thing you are trying to achieve is - "lazy deoptimization".
There were some related V8 papers/presentations.

Yeah, but actually doing it properly is fully overkill for our needs :-)

dstogov · 2024-09-11T06:23:21Z

ext/opcache/jit/zend_jit_vm_helpers.c

+			// First not-skipped op
+			zend_op *opline = func->op_array.opcodes;
+			if (!(func->op_array.fn_flags & ZEND_ACC_HAS_TYPE_HINTS)) {
+				while (opline->opcode == ZEND_RECV || opline->opcode == ZEND_RECV_INIT) {
+					opline++;
+				}
+			}
+			if (jit_extension && ZEND_OP_TRACE_INFO(opline, jit_extension->offset)->trace_flags & ZEND_JIT_TRACE_BLACKLISTED) {


You may use jit_extension->func_info.flags check to stop tracing.
Just remove ZEND_FUNC_JIT_ON_HOT_TRACE flag.

dstogov · 2024-09-11T06:27:51Z

ext/opcache/jit/zend_jit_vm_helpers.c

+				if (jit_extension && ZEND_OP_TRACE_INFO(opline, offset)->trace_flags & ZEND_JIT_TRACE_BLACKLISTED) {
+					stop = ZEND_JIT_TRACE_STOP_BLACK_LIST;
+					break;
+				}
+


you should check jit_extension->func_info.flags not the opline flags.

dstogov · 2024-09-11T06:32:46Z

ext/opcache/jit/zend_jit_trace.c

+		return;
+	}
+
+	zend_jit_stop_persistent_op_array(op_array);


This should be done with lock and SHM unprotectd.

dstogov · 2024-09-11T06:36:54Z

ext/opcache/jit/zend_jit_trace.c

+	// First not-skipped op
+	zend_op *opline = op_array->opcodes;
+	if (!(op_array->fn_flags & ZEND_ACC_HAS_TYPE_HINTS)) {
+		while (opline->opcode == ZEND_RECV || opline->opcode == ZEND_RECV_INIT) {
+			opline++;
+		}
+	}
+
+	zend_jit_blacklist_root_trace(opline, jit_extension->offset);


Reset ZEND_FUNC_JIT_ON_HOT_TRACE in jit_extension->func_info.flags instead.

bwoebi · 2024-09-16T17:49:20Z

@dstogov Is the current iteration more in line with your expectations? I'd still like to retain the zend_jit_blacklist_function() as an API to use via dlsym() - as it's naturally much less overhead to directly pass the op_array than having to fetch it somehow.

dstogov

This looks better, but probably the big part of the patch is not necessary now. Se the comments.
I don't care about dlsym() usage a lot.

dstogov · 2024-09-19T07:24:24Z

ext/opcache/jit/zend_jit_trace.c

@@ -7656,6 +7656,22 @@ static void zend_jit_blacklist_root_trace(const zend_op *opline, size_t offset)
 	zend_shared_alloc_unlock();
 }

+ZEND_EXT_API void zend_jit_blacklist_function(zend_op_array *op_array) {
+	zend_jit_op_array_trace_extension *jit_extension = (zend_jit_op_array_trace_extension *)ZEND_FUNC_INFO(op_array);
+	if (!jit_extension || !(jit_extension->func_info.flags & ZEND_JIT_ON_HOT_TRACE)) {


Is it possible to support other JIT triggers? (function JIT)

I have no idea how that works. Gladly, I'd do it, if I knew how.

This is relatively easy for functions that are not JIT-ed yet. See zend_jit_op_array() in zend_jit.c. It creates different zend_jit_op_array*_extension structures for different triggers. The trigger may be determined through corresponding flag in jit_extension->func_info.flags``. So for different triggers you should just restore the original handlers. This should work for ZEND_JIT_ON_FIRST_EXEC, ZEND_JIT_ON_PROF_REQUEST (only one handler should be reverted), and ZEND_JIT_ON_HOT_COUNTERS (all handlers should be reverted).

For ZEND_JIT_ON_SCRIPT_LOAD trigger and op_arrays JIT-ed because of ZEND_JIT_ON_FIRST_EXEC, ZEND_JIT_ON_PROF_REQUEST and ZEND_JIT_ON_HOT_COUNTERS we should use zend_jit_op_array_extension structure with ZEND_JIT_ON_SCRIPT_LOAD flag and single entry handler. It's possible to create it in zend_real_jit_func or zend_jit.

Anyway, this looks not trivial and may delay this PR for edges.

dstogov · 2024-09-19T07:31:39Z

ext/opcache/jit/zend_jit_trace.c

+	zend_shared_alloc_lock();
+	SHM_UNPROTECT();


ZendAccelerator.c first do this in opposite order. First SHM_UNPROTECT() then zend_shared_alloc_lock().
This was done to allow usage of spin-locks. I'm not sure if this makes any difference, but it's better to use the same order everywhere.

I'm copying the logic from zend_jit_blacklist_root_trace where it's first alloc_lock, then SHM_UNPROTECT?

OK. This is not a big problem.

dstogov · 2024-09-19T07:42:13Z

ext/opcache/jit/zend_jit_vm_helpers.c

+					// First not-skipped op
+					zend_op *opline = func->op_array.opcodes;
+					if (!(func->op_array.fn_flags & ZEND_ACC_HAS_TYPE_HINTS)) {
+						while (opline->opcode == ZEND_RECV || opline->opcode == ZEND_RECV_INIT) {
+							opline++;
+						}
+					}
+					if (jit_extension && ZEND_OP_TRACE_INFO(opline, jit_extension->offset)->trace_flags & ZEND_JIT_TRACE_BLACKLISTED) {
+						stop = ZEND_JIT_TRACE_STOP_BLACK_LIST;


This code is not necessary anymore.
You may stop tracing checking (jit_extension->func_info.flags & ZEND_FUNC_JIT_ON_HOT_TRACE).
If you need to make difference between stop = ZEND_JIT_TRACE_STOP_INTERPRETER and ZEND_JIT_TRACE_STOP_BLACK_LIST you may add another flag into jit_extension->func_info.flags.

I'm not completely sure if this is the right place to stop tracing.
Now you prevent code-generation for the call sequence INIT_FCALL, SEND*, DO_FCALL

I think it should be stopped at the point where we record ZEND_JIT_TRACE_ENTER.
Actually this should already work out of the box because of ZEND_FUNC_JIT_ON_HOT_TRACE check at

php-src/ext/opcache/jit/zend_jit_vm_helpers.c

Line 944 in 1b9568d

|| UNEXPECTED(!(jit_extension->func_info.flags & ZEND_FUNC_JIT_ON_HOT_TRACE))) {

I'm not completely sure...
This should also take care about termination at the point where we record ZEND_JIT_TRACE_BACK, that you missed previously.

Probably you may safely remove your changes in zend_jit_trace_execute() and zend_jit_trace_record_fake_init_call_ex()

I wanted to do the same changes here than around lines 515 (see above), but forgot about it. Pushed the correction.

dstogov

I don't see critical problems any more.

Signed-off-by: Bob Weinand <[email protected]>

dstogov · 2024-09-24T10:44:10Z

ext/opcache/zend_accelerator_module.c

@@ -924,6 +925,21 @@ ZEND_FUNCTION(opcache_invalidate)
 	}
 }

+/* {{{ Prevents JIT on function. Call it before the first invocation of the given function. */
+ZEND_FUNCTION(opcache_jit_blacklist)


It would be good to have a least one test for this function.
I suppose, it can't be called at this moment... :)

There is a big question about usability of this API.
It can prevent and revert JIT-ing of already cached functions, but it can't disable setting JIT counters at the first place.

It can be called, but not really tested for its functionality, unless you have a way how to check whether a function is actually executed under jit without looking at it in gdb.
So added a very basic test, just calling it in a JIT scenario, to make sure it doesn't crash. Manually verified it works, too.

Oh, sorry, my mistake. I forgot about automatic registration through opcache.stub.php.

And add basic test for opcache_jit_blacklist. Signed-off-by: Bob Weinand <[email protected]>

bwoebi requested review from dstogov and iluuu1994 as code owners August 23, 2024 18:39

github-actions bot added the Extension: opcache label Aug 23, 2024

dstogov requested changes Aug 26, 2024

View reviewed changes

bwoebi force-pushed the jit-function-blacklist-api branch 3 times, most recently from c3f7879 to ca1ee03 Compare September 4, 2024 11:04

dstogov requested changes Sep 11, 2024

View reviewed changes

bwoebi force-pushed the jit-function-blacklist-api branch from ca1ee03 to c170512 Compare September 16, 2024 17:03

bwoebi requested a review from kocsismate as a code owner September 16, 2024 17:03

dstogov reviewed Sep 19, 2024

View reviewed changes

bwoebi force-pushed the jit-function-blacklist-api branch from d1ea7b8 to 6ce13ce Compare September 20, 2024 12:39

dstogov approved these changes Sep 24, 2024

View reviewed changes

bwoebi added 6 commits September 24, 2024 12:11

Add API to exempt function from being JITed

e0a40f4

Signed-off-by: Bob Weinand <[email protected]>

Add check for blacklisted traces when calling functions

d4fdaa6

Signed-off-by: Bob Weinand <[email protected]>

Allow EG(vm_interrupt) in observer start handler to leave JIT

8e067ce

Signed-off-by: Bob Weinand <[email protected]>

Also reset hot handlers

e9bb403

Signed-off-by: Bob Weinand <[email protected]>

Use ZEND_FUNC_JIT_ON_HOT_TRACE flag

4349069

Signed-off-by: Bob Weinand <[email protected]>

Fix after rebase

498f6ae

Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the jit-function-blacklist-api branch from 6ce13ce to 4a9d65e Compare September 24, 2024 10:11

dstogov reviewed Sep 24, 2024

View reviewed changes

bwoebi force-pushed the jit-function-blacklist-api branch from 4a9d65e to 29e64e5 Compare September 24, 2024 10:53

Fixup remaining place in zend_jit_vm_helpers.c

6e90810

And add basic test for opcache_jit_blacklist. Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the jit-function-blacklist-api branch from 29e64e5 to 6e90810 Compare September 24, 2024 11:12

Add NEWS/UPGRADING entries

ef5500f

bwoebi changed the title ~~Add API to exempt function from being JITed~~ Add API to exempt function from being traced in JIT Sep 24, 2024

bwoebi merged commit 654b787 into php:master Sep 24, 2024
8 of 10 checks passed

jrfnl mentioned this pull request Oct 7, 2024

PHP 8.4 | NewFunctions: detect use of new opcache_jit_blacklist() function PHPCompatibility/PHPCompatibility#1789

Merged

Add API to exempt function from being traced in JIT #15559

Add API to exempt function from being traced in JIT #15559

Uh oh!

Conversation

bwoebi commented Aug 23, 2024

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Aug 26, 2024

Uh oh!

dstogov commented Aug 28, 2024

Uh oh!

iluuu1994 commented Aug 28, 2024

Uh oh!

bwoebi commented Sep 10, 2024

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi commented Sep 16, 2024

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Sep 12, 2024 •

edited

Loading

bwoebi Sep 24, 2024 •

edited

Loading