Page MenuHomePhabricator

[xray] Function coverage groups
ClosedPublic

Authored by ianlevesque on Sep 18 2020, 3:44 PM.

Details

Summary

Add the ability to selectively instrument a subset of functions by dividing the functions into N logical groups and then selecting a group to cover. By selecting different groups over time you could cover the entire application incrementally with lower overhead than instrumenting the entire application at once.

Diff Detail

Event Timeline

ianlevesque created this revision.Sep 18 2020, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 3:44 PM
ianlevesque requested review of this revision.Sep 18 2020, 3:44 PM
dberris added inline comments.Sep 19 2020, 7:53 PM
clang/lib/CodeGen/CodeGenFunction.cpp
817

I'm a little concerned that this is randomly determined. Could we think about using a function attribute instead that opts functions into groups by name? Or is the intent to have "sampling" in the coverage information instead of full coverage?

ianlevesque added inline comments.Sep 19 2020, 9:06 PM
clang/lib/CodeGen/CodeGenFunction.cpp
817

The intention is to have something deterministic across builds but yeah sampled-ish across the entire codebase. By iterating over the groups with different builds we are hoping to cover every function eventually.

The idea seems fine.

By selecting different groups over time you could cover the entire application incrementally with lower overhead than instrumenting the entire application at once.

How large the overhead is? This is somewhat surprising to me.

clang/include/clang/Driver/Options.td
1343

Joined.

Separate is for a space between the option name and its value.

1345

It is "N groups", not "N functions".

clang/lib/CodeGen/CodeGenFunction.cpp
814

For one group, the branch is skipped, which does not seem correct

815

const ArrayRef<uint8_t> FuncName(..., ...)

820

Omit braces around one-line simple statements.

clang/test/CodeGen/xray-function-groups.cpp
5

You can omit -x c++ -std=c++11 . They are insignificant.

Then consider packing more options on one line to reduce the total number of lines.

kyulee added inline comments.Sep 20 2020, 11:14 AM
clang/lib/CodeGen/CodeGenFunction.cpp
814

Should we check or assert XRaySelectedFunctionGroups > 0 && XRaySelectedFunctionGroup < XRaySelectedFunctionGroups or the former at minimum?

MaskRay added inline comments.Sep 20 2020, 11:26 AM
clang/lib/CodeGen/CodeGenFunction.cpp
814

Such checks are usually done in Driver. In CodeGen, as long as it doesn't crash, there is no need imposing stricter checks.

ianlevesque marked 7 inline comments as done.Sep 21 2020, 4:12 PM
ianlevesque added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
814

If there is only 1 group (the default value), then all functions that would be otherwise instrumented are. So there should be no need to do anything.

ianlevesque marked 3 inline comments as done.Sep 21 2020, 4:14 PM

Address code review feedback

How large the overhead is? This is somewhat surprising to me.

It is the binary size overhead, not the runtime overhead, that we are limited on (when deployed on Android).

This is ready for another review, I think I addressed everything.

MaskRay added inline comments.Sep 22 2020, 7:58 PM
clang/lib/Frontend/CompilerInvocation.cpp
1136

Errors here are not necessary. The driver has reported errors.

Remove extraneous parameter validations.

I think it looks good to me. @MaskRay Any further feedback on this?

MaskRay added a comment.EditedSep 24 2020, 10:08 AM

I think it looks good to me. @MaskRay Any further feedback on this?

This looks good from my viewpoint. One thing remains unanswered (https://reviews.llvm.org/D87953#2284071) is how the overhead is so large that you want to have multiple groups.
Deploying N versions of an executable can bring more challenges to the build system and deployment system.
I am also concerned about some internal functions whose names are optional (in ELF symbol table they don't really need a name).

I'd want to know how you weigh the tradeoff and decide to go this route.

This is a larger stuff that I want @dberris to sign off.

clang/lib/CodeGen/CodeGenFunction.cpp
815

You can use some variants of makeArrayRef

Thanks @MaskRay - I tried to answer that question in https://reviews.llvm.org/D87953#2286430. At present we are deploying instrumentation to an arbitrary subset of our application using the instruction threshold. I would like to make the selection of how many and which functions more deterministic, and be able to instrument different subsets over time. The overhead we are concerned with is purely binary size as we are deploying to Android devices. We are using features from my previous XRay patches to omit the function index already, but the sheer number of sleds and size of the associated xray_instr_map are the limiting factor of how much we can instrument in any given app release. For our use case it is fine to gradually over a period of weeks work our way across the entire app group by group.

Thanks @MaskRay - I tried to answer that question in https://reviews.llvm.org/D87953#2286430. At present we are deploying instrumentation to an arbitrary subset of our application using the instruction threshold. I would like to make the selection of how many and which functions more deterministic, and be able to instrument different subsets over time. The overhead we are concerned with is purely binary size as we are deploying to Android devices. We are using features from my previous XRay patches to omit the function index already, but the sheer number of sleds and size of the associated xray_instr_map are the limiting factor of how much we can instrument in any given app release. For our use case it is fine to gradually over a period of weeks work our way across the entire app group by group.

Thanks for clarification. It is the size overhead, instead of the runtime overhead:) This makes sense. You probably want to enhance the test to check internal linkage (e.g. static functions). You can change bar or yarr instead of introducing new functions.

I probably have asked too much... but it'd be nice if you also add some documentation to llvm/docs/XRay*.rst That helps users :)

There's two tensions I'm trying to resolve in my mind with this patch:

  • Is there a way to make this more deterministic, instead of using a random sampling mechanism based on the name?
  • This looks like it's a subset of a more general facility to group functions by "module" or some logical grouping mechanism. Will this be superseded by a feature that allows for example to provide filters in the driver to define groupings? If so, wouldn't something like that be more flexible for your needs (group names in the IR)?

I can update the docs @MaskRay, not a problem. I'll tweak the test a little too per your comment.

@dberris I think if we wanted to control it in a non-'random' (the crc32 was chosen so it would be consistent even though it's not really predictable up front) way we'd have to emit some lists of function attributes and use the -fxray-attr-list=FILE option to really dial that in. If we wanted entire modules we could have the build system apply xray's flags to each module differently, but we are unlikely to want to instrument every function of a module at the same time so we are back to having to use instruction thresholds to cut down on it. Ultimately I think those two suggestions are legitimate alternative use cases but don't, for what we're trying to do, replace this strategy.

dberris accepted this revision.Sep 24 2020, 6:38 PM

Okay, I think I understand the motivation a little bit more now (i.e. sampling seems to be a useful thing to do anyway, regardless of whether we have the other facilities).

This revision is now accepted and ready to land.Sep 24 2020, 6:38 PM

Add static function to the test case, update documentation.

Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 6:39 PM
This revision was automatically updated to reflect the committed changes.