Skip to content

Stop JIT hot spot counting - #9343

Merged
dstogov merged 1 commit into
php:masterfrom
wxue1:stop_jit_hot_counting
Aug 24, 2022
Merged

dstogov merged 1 commit into
php:masterfrom
wxue1:stop_jit_hot_counting

Conversation

@wxue1

@wxue1 wxue1 commented Aug 15, 2022

Copy link
Copy Markdown

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 xue1.wang@intel.com

Comment thread ext/opcache/jit/zend_jit_trace.c Outdated
op_array->opcodes[i].handler = jit_extension->trace_info[i].orig_handler;
SHM_PROTECT();
}
zend_shared_alloc_unlock();

Copy link
Copy Markdown
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
Copy Markdown
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 <xue1.wang@intel.com>
@wxue1
wxue1 force-pushed the stop_jit_hot_counting branch from fbfa5c0 to 11946e9 Compare August 16, 2022 07:10
@dstogov

dstogov commented Aug 18, 2022

Copy link
Copy Markdown
Member

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

@dstogov dstogov left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

identation

@wxue1

wxue1 commented Aug 23, 2022 •

Copy link
Copy Markdown
Author

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

dstogov commented Aug 23, 2022

Copy link
Copy Markdown
Member

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

stkeke commented Aug 24, 2022

Copy link
Copy Markdown
Contributor

@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