Skip to content

Stop JIT hot spot counting #9343

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

Merged
merged 1 commit into from
Aug 24, 2022
Merged

Stop JIT hot spot counting #9343

merged 1 commit into from
Aug 24, 2022

Conversation

wxue1
Copy link
Contributor

@wxue1 wxue1 commented Aug 15, 2022

When max_root_trace is reached, JIT in tracing mode will not
compile any new code for root trace and side trace, but
counting hot code is still going on. This patch stops counting
as soon as possible by replacing counter handler with
original handler, which increases 1.5% performance.

Signed-off-by: Wang, Xue [email protected]

op_array->opcodes[i].handler = jit_extension->trace_info[i].orig_handler;
SHM_PROTECT();
}
zend_shared_alloc_unlock();
Copy link
Member

Choose a reason for hiding this comment

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

Should this be outside the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be outside the loop?

Yes, I will update

When max_root_trace is reached, JIT in tracing mode will not
compile any new code for root trace and side trace, but
counting hot code is still going on. This patch stops counting
as soon as possible by replacing counter handler with
original handler, which increases 1.5% performance.

Signed-off-by: Wang, Xue <[email protected]>
@wxue1 wxue1 force-pushed the stop_jit_hot_counting branch from fbfa5c0 to 11946e9 Compare August 16, 2022 07:10
@dstogov
Copy link
Member

dstogov commented Aug 18, 2022

The idea looks great. I'll take a deeper look into implementation on next week, when return back from vacation.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

The idea is good. However, I don't see big benefits against just lazy blacklisting. Please explain, I may miss something.

Anyway, this idea may be used to stop counting after some signal (e.g. after warm up).

}
if (jit_extension->trace_info[i].trace_flags &
(ZEND_JIT_TRACE_START_LOOP | ZEND_JIT_TRACE_START_ENTER | ZEND_JIT_TRACE_START_RETURN)) {
op_array->opcodes[i].handler = jit_extension->trace_info[i].orig_handler;
Copy link
Member

Choose a reason for hiding this comment

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

identation

Comment on lines +7155 to +7157
if (!func_info) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

identation

@wxue1
Copy link
Contributor Author

wxue1 commented Aug 23, 2022

The idea is good. However, I don't see big benefits against just lazy blacklisting. Please explain, I may miss something.

Anyway, this idea may be used to stop counting after some signal (e.g. after warm up).

Yeah in our workload, all hotspot codes have been jitted in the warmup phase and those counter of JIT tracing is not necessary anymore after warmup. Actually, The JIT tracing counter wrapper functions(hybrid_func_trace_counter, hybrid_ret_trace_counter and hybrid_loop_trace_counter) bring numerous branch mispredictions.

The lazy blacklisting is still counting before becoming hot. So it is better to stop counting as soon as possible.

@dstogov
Copy link
Member

dstogov commented Aug 23, 2022

The lazy blacklisting is still counting before becoming hot. So it is better to stop counting as soon as possible.

Right. This is the key feature 👍
I'll think a bit more and merge this into master by the end of the week.

@dstogov dstogov merged commit 9f926d6 into php:master Aug 24, 2022
@stkeke
Copy link
Contributor

stkeke commented Aug 24, 2022

@dstogov Thanks Dmitry. We will keep investing energy and squeezing performance for PHP...

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.

5 participants