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

Call counts in Spacetime #1180

Merged
merged 2 commits into from Jun 16, 2017
Merged

Call counts in Spacetime #1180

merged 2 commits into from Jun 16, 2017

Conversation

@mshinwell
Copy link
Contributor

mshinwell commented May 24, 2017

This patch provides for the counting of:

  • the number of direct calls from OCaml to OCaml functions;
  • the number of direct calls from OCaml to external functions; and
  • the number of indirect calls from OCaml to OCaml functions

during the execution of a program running with Spacetime instrumentation. This helps, in particular, to identify indirect calls that might be able to be turned into direct calls (for example with an inlining annotation on a functor) to realise a performance gain. It should also help identify targets for the adding of inlining annotations (for example when there are many direct calls to some small function).

As when viewing allocations using Spacetime, the backtrace is available for each direct and indirect call. Two pull requests have been submitted for the viewer and associated library:

  1. lpw25/spacetime_lib#5
  2. lpw25/prof_spacetime#16

This functionality requires extra memory and a small amount of overhead over the existing Spacetime memory profiling. It also generates different code when compiling OCaml functions so must be enabled by a compiler configuration option. Profiles produced using a compiler with this patch but call counts disabled can be decoded by existing versions of the viewer. The version of the viewer supporting call counts can read profiles that were created both with and without call counts.

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 24, 2017

You appear to have locked your PRs for edits: for Windows, you need this commit: dra27@e3e8d04

Copy link
Contributor

dra27 left a comment

Only a minor style question. Do we really need to be updating copyright headers with each change? (not sure that it's what's done elsewhere)?

incr index_within_node;
if Config.spacetime_call_counts then begin
incr index_within_node
end

This comment has been minimized.

Copy link
@dra27

dra27 May 24, 2017

Contributor

Why the begin and end?

This comment has been minimized.

Copy link
@mshinwell

mshinwell May 25, 2017

Author Contributor

To guard against if-then-else errors. I put them everywhere now unless the conditional is straightforward and fits entirely on a single line.

@@ -220,7 +225,24 @@ let code_for_call ~node ~callee ~is_tail ~label =
(hard) node hole pointer register immediately before the call.
(That move is inserted in [Selectgen].) *)
match callee with
| Direct _callee -> Cvar place_within_node
| Direct _callee ->
if Config.spacetime_call_counts then begin

This comment has been minimized.

Copy link
@dra27

dra27 May 24, 2017

Contributor

Similarly (and on the else)?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

mshinwell commented May 25, 2017

I think copyright headers should probably be updated in cases such as these where the authorship is clear, although @xavierleroy would have the final word on such things.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

mshinwell commented May 25, 2017

@dra27 I've merged your patch for Windows, thanks.

@@ -131,12 +133,17 @@ typedef struct {
} allocation_point;

typedef struct {
value callee_node;
value call_count;

This comment has been minimized.

Copy link
@lpw25

lpw25 Jun 9, 2017

Contributor

Should this be guarded by WITH_SPACETIME_CALL_COUNTS? It is used in spacetime.c which suggests it should, but it is also used in spacetime_offline.c` which suggests it shouldn't.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Jun 12, 2017

Author Contributor

Guarding this is going to cause trouble in the decoding code, I think, which is supposed to cope whether or not the profile contains call counts. Luckily though the size of allocation_point is already larger than the size of call_point and they are both in a union together, so this does not increase memory consumption. So I don't think anything needs changing here.

@@ -568,7 +580,8 @@ static c_node* allocate_c_node(void)

node->gc_header =
Make_header(sizeof(c_node)/sizeof(uintnat) - 1, C_node_tag, Caml_black);
node->data.callee_node = Val_unit;
node->data.call.callee_node = Val_unit;
node->data.call.call_count = Val_long(0);

This comment has been minimized.

Copy link
@lpw25

lpw25 Jun 9, 2017

Contributor

Should this be protected by WITH_SPACETIME_CALL_COSTS?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Jun 12, 2017

Author Contributor

I think this should probably stay unguarded, since there is a named struct field at this point.

@lpw25 lpw25 added the approved label Jun 16, 2017
@mshinwell mshinwell merged commit 9683393 into ocaml:trunk Jun 16, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche gasche mentioned this pull request Oct 9, 2017
@vicuna vicuna mentioned this pull request Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.