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

lj_record.c: Record IFUNC/IFUNCV the same as FUNC/FUNCV #1244

Merged
merged 1 commit into from Dec 12, 2017

Conversation

Projects
None yet
2 participants
@lukego
Member

lukego commented Nov 8, 2017

Record calls to blacklisted functions the same way as calls to
uncompiled functions. This allows for the possibility that a
blacklisted function can be successfully recorded as part of a larger
trace.

This is specifically intended to fix the problem where a tiny utility
function somewhere, such as ntohs, is not able to be compiled in
isolation for some reason and that ends up preventing all of its
callers from compiling too.

This has tended to happen for obscure reasons like "the utility
function ends with a tail-call to a C library routine" and with this
patch we should not need to worry about such minutiae anymore. (We
should be able to retire the "wrap in parenthesis to prevent a
tailcall" trick that some people are familiar with.)

More background:

lj_record.c: Record IFUNC/IFUNCV the same as FUNC/FUNCV
Record calls to blacklisted functions the same way as calls to
uncompiled functions. This allows for the possibility that a
blacklisted function can be successfully recorded as part of a larger
trace.

This is specifically intended to fix the problem where a tiny utility
function somewhere, such as `ntohs`, is not able to be compiled in
isolation for some reason and that ends up preventing all of its
callers from compiling too.

This has tended to happen for obscure reasons like "the utility
function ends with a tail-call to a C library routine" and with this
patch we should not need to worry about such minutiae anymore. (We
should be able to retire the "wrap in parenthesis to prevent a
tailcall" trick that some people are familiar with.)

More background:

- How the trace recorder treats function calls: raptorjit/raptorjit#118.
- Why this can be a problem: raptorjit/raptorjit#119.
- More on this specific solution: raptorjit/raptorjit#120.
@lukego

This comment has been minimized.

Show comment
Hide comment
@lukego

lukego Nov 8, 2017

Member

I am running a performance regression test on this now. I would be very interested to hear if somebody can confirm that this change renders "the parenthesis trick" obsolete i.e. the function with the tailcall will still be blacklisted but we won't care because the calls will all be successfully inlined anyway (cc @alexandergall @eugeneia @wingo.)

Member

lukego commented Nov 8, 2017

I am running a performance regression test on this now. I would be very interested to hear if somebody can confirm that this change renders "the parenthesis trick" obsolete i.e. the function with the tailcall will still be blacklisted but we won't care because the calls will all be successfully inlined anyway (cc @alexandergall @eugeneia @wingo.)

@eugeneia

This comment has been minimized.

Show comment
Hide comment
@eugeneia

eugeneia Nov 10, 2017

Member

I have started a Hydra jobset that compares my base branch (vita), a version that includes attempts to manually inhibit tail calls to C functions (vita-stop-tco), a version with this PR merged (vita-record-blacklisted), and finally a version with both of the latter (vita-stop-tco+record-blacklisted, or "st+rb" in the report). Results will be here:

https://hydra.snabb.co/eval/7590

Member

eugeneia commented Nov 10, 2017

I have started a Hydra jobset that compares my base branch (vita), a version that includes attempts to manually inhibit tail calls to C functions (vita-stop-tco), a version with this PR merged (vita-record-blacklisted), and finally a version with both of the latter (vita-stop-tco+record-blacklisted, or "st+rb" in the report). Results will be here:

https://hydra.snabb.co/eval/7590

@eugeneia

This comment has been minimized.

Show comment
Hide comment
@eugeneia

eugeneia Nov 11, 2017

Member

https://hydra.snabb.co/eval/7621#tabs-still-succeed

I increased the number of iterations a bit, Tukey’s test still doesn’t see any differences. This may be very well because the outliers don’t impact the average result much.

To me it looks like record-blacklisted does have fewer "teeth" (i.e. dramatically bad traces) in the linegraph: https://hydra.snabb.co/build/2473885/download/2/report.html#line-graph

It also "looks" like the optimizations cancel each other out... no definitive answer from me here!

Member

eugeneia commented Nov 11, 2017

https://hydra.snabb.co/eval/7621#tabs-still-succeed

I increased the number of iterations a bit, Tukey’s test still doesn’t see any differences. This may be very well because the outliers don’t impact the average result much.

To me it looks like record-blacklisted does have fewer "teeth" (i.e. dramatically bad traces) in the linegraph: https://hydra.snabb.co/build/2473885/download/2/report.html#line-graph

It also "looks" like the optimizations cancel each other out... no definitive answer from me here!

eugeneia added a commit to eugeneia/snabb that referenced this pull request Dec 10, 2017

@eugeneia eugeneia merged commit 1862084 into snabbco:master Dec 12, 2017

1 of 2 checks passed

davos-eugeneia/snabb-nfv-test-vanilla Linux davos 4.4.11 x86_64 Intel(R) Xeon(R) CPU E5-2603 v2 @ 1.80GHz / eugeneia/snabb-nfv-test-vanilla
Details
SnabbDoc Documentation as single HTML file
Details

@wingo wingo referenced this pull request May 14, 2018

Merged

Prepare v3.1.10 release #1068

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment