Skip to content
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

Support CPU profiling sections of code #3971

Merged
merged 24 commits into from May 6, 2019

Conversation

Projects
None yet
8 participants
@breeswish
Copy link
Member

commented Dec 24, 2018

Signed-off-by: Breezewish breezewish@pingcap.com

What have you changed? (mandatory)

We usually need to find out why specific sections of code are slow. However, sometimes it cannot be done via simple perf, i.e. when there is a bootstrap. This PR provides facility to programmatically start or stop CPU profiling. In this way, we can start profiling before our interested code begins and stop profiling after it ends.

We can explore more interesting usages in the future, i.e. toggle profiling dynamically via signals, or environment variables. Future PRs are welcome!

The manual is in the code. I paste it here for easy reading:


Profile a part of the code using CPU Profiler from gperftools or Callgrind.

Requirements

  1. Linux

    Other OS may also work however, not tested.

  2. gperftools

    You can follow its INSTALL manual.
    Roughly the instructions are the following:

    1. Download packages from release
    2. Run ./configure
    3. Run make install

Usage

profiler::start("./app.profile");
some_complex_code();
profiler::stop();

Then, compile the code with profiling feature enabled and run the code with environment
variable TIKV_PROFILE=1.

By default, a profile called app.profile will be generated by CPU Profiler.
You can then analyze the profile using pprof.

If the application is running in Callgrind, a Callgrind profile dump will be generated instead.
Notice that you should run Callgrind with command line option --instr-atstart=no, e.g.:

TIKV_PROFILE=1 valgrind --tool=callgrind --instr-atstart=no ./my_example

Also see examples/prime.rs.

Support profiling sections of code
Signed-off-by: Breezewish <breezewish@pingcap.com>
@siddontang

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2018

Do we need to install gperftools at first?

@breeswish breeswish force-pushed the breeswish:__profiler branch from 28285d6 to 5d8d429 Dec 24, 2018

Support Callgrind
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:__profiler branch from 5d8d429 to 9ddf5e9 Dec 24, 2018

@tikv tikv deleted a comment from zhouqiang-cl Dec 24, 2018

@tikv tikv deleted a comment from siddontang Dec 24, 2018

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2018

@siddontang Yes. I copied the manual from code to the PR description now. I also updated the code to support Callgrind (so that we can profile cache hit etc).

breeswish added some commits Dec 24, 2018

Make rustfmt and clippy happy
Signed-off-by: Breezewish <breezewish@pingcap.com>
Fix doc test
Signed-off-by: Breezewish <breezewish@pingcap.com>
@zhouqiang-cl

This comment was marked as outdated.

Copy link
Collaborator

commented Dec 25, 2018

/rebuild

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Dec 25, 2018

So if the user doesn't install perftools, can it build TiKV now?

I suggest you introduce valgrind in another PR.

@Connor1996 Connor1996 added the C: Perf label Dec 25, 2018

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Dec 25, 2018

@siddontang Yes. Only when profiling feature is enabled, gperftools is required. As you can see, it builds normally on Circle CI and our own Jenkins CI.

The support of Valgrind is only 4 lines of code. It should not be a big problem.

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

do we need to install vagrind manally too?

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

IMO, if we build with this feature, we don't need using environment forcibly to control it. Only calling start and stop is enough.

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Dec 26, 2018

@siddontang Don't need to install valgrind to build it.

Ok

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

Updated. No need to set "TIKV_PROFILE=1" now.

breeswish added some commits Jan 2, 2019

start profiling without env variables
Signed-off-by: Breezewish <breezewish@pingcap.com>
Merge remote-tracking branch 'origin/master' into __profiler
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish force-pushed the breeswish:__profiler branch from b660fc4 to 1827120 Jan 2, 2019

Fix lock
Signed-off-by: Breezewish <breezewish@pingcap.com>

@breeswish breeswish referenced this pull request Apr 7, 2019

Closed

Link tcmalloc statically #4471

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@brson PTAL for this~ Thanks a lot!

Merge remote-tracking branch 'origin/master' into __profiler
Signed-off-by: Breezewish <breezewish@pingcap.com>
@brson

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

This is very cool @breeswish.

I guess that the idea is to insert start and stop temporarily and then remove them when done? Otherwise there's a lot of deadlock potential.

One nice change would be to change the profiler manifest to make all the dependencies optional (particularly the profiling deps), and have the profiling target enable them. That would save the download, build, and link of the profiler crates.

Is there anything holding up landing this?

Maybe docs - like the allocator configuration this seems like something that should be surfaced in a developer's guide. Not necessary for this PR though.

Merge remote-tracking branch 'origin/master' into __profiler
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@brson @kennytm Thanks a lot for reviewing! I have updated my Cargo.toml to make these dependencies optional (not sure it there are better ways). Also updated docs for MacOS instructions.

I guess that the idea is to insert start and stop temporarily and then remove them when done? Otherwise there's a lot of deadlock potential.

Yes. A better way maybe, provide interfaces to trigger a start and stop it moment later so that a profile can be generated. This does not introduce runtime costs when profiling is not started. The interface can be our status server like #4444

Is there anything holding up landing this?

Nop, it's just lack of reviewers previously :)

@breeswish breeswish requested a review from brson Apr 12, 2019

@breeswish breeswish requested a review from zhangjinpeng1987 Apr 12, 2019

Use tikv_alloc
Signed-off-by: Breezewish <breezewish@pingcap.com>
@siddontang

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

@brson

I think we can advance this PR now.
But here I prefer using HTTP API to control profile.

@breeswish

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

@siddontang We can have another PR that utilize the interface via HTTP API.

breeswish added some commits Apr 15, 2019

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

PTAL @brson

@brson

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@siddontang acknowledged that we want an HTTP interface, and that this is a good start.

@brson brson added the S: LGT1 label Apr 26, 2019

@brson

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

lgtm

@BusyJay
Copy link
Contributor

left a comment

Can gperftools be installed automatically?

publish = false

[features]
profiling = ["lazy_static", "cpuprofiler", "callgrind", "valgrind_request"]

This comment has been minimized.

Copy link
@BusyJay

BusyJay Apr 26, 2019

Contributor

Can it compile on Windows without the feature enabled?

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 28, 2019

Author Member

It should work though I don't have a Windows machine. I changed it to [target.'cfg(linux)'.dependencies] and it can compile on my MacOS.

//! Also see `examples/prime.rs`.

#[allow(unused_extern_crates)]
extern crate tikv_alloc;

This comment has been minimized.

Copy link
@BusyJay

BusyJay Apr 26, 2019

Contributor

Why use tikv_alloc?

This comment has been minimized.

Copy link
@brson

brson Apr 26, 2019

Contributor

tikv_alloc generally needs to be linked into every crate that doesn't link to tikv, so that tests and benches of that crate use tikv's allocator. I don't think this extern crate statement is needed though as long as the dependency exists.

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 26, 2019

Author Member

CI will fail if alloc is not linked

This comment has been minimized.

Copy link
@brson

brson Apr 28, 2019

Contributor

Ah, right. There are tests that all binaries contain jemalloc.

Show resolved Hide resolved components/profiler/src/lib.rs Outdated
Show resolved Hide resolved components/profiler/src/profiler_linux.rs Outdated
[target.'cfg(unix)'.dependencies]
lazy_static = { version = "1.2.0", optional = true }
cpuprofiler = { version = "0.0.3", optional = true }
callgrind = { version = "1.1.0", optional = true }

This comment has been minimized.

Copy link
@BusyJay

BusyJay Apr 26, 2019

Contributor

Is valgrind a useful use case? I think most of the time only cpuprofiling is used.

This comment has been minimized.

Copy link
@breeswish

breeswish Apr 26, 2019

Author Member

valgrind is very useful for micro benchmark, which can report precise amount of function calls, as well as precise (emulated) cache hit.

This comment has been minimized.

Copy link
@brson

brson Apr 28, 2019

Contributor

I'm also hoping that this crate can be extended into a general purpose profiling module that can be published for the community, and ultimately fulfill the various requirements of the Go profiling tools. More profiler options in that case seems good.

breeswish added some commits Apr 28, 2019

Address comments about the returning value
Signed-off-by: Breezewish <breezewish@pingcap.com>
Upgrade dependency
Signed-off-by: Breezewish <breezewish@pingcap.com>

breeswish and others added some commits Apr 29, 2019

@siddontang
Copy link
Contributor

left a comment

LGTM

@brson

brson approved these changes May 6, 2019

Copy link
Contributor

left a comment

LGTM

@breeswish breeswish merged commit 68802bb into tikv:master May 6, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:__profiler branch May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.