Conversation
|
You appear to have locked your PRs for edits: for Windows, you need this commit: dra27@e3e8d04 |
dra27
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
To guard against if-then-else errors. I put them everywhere now unless the conditional is straightforward and fits entirely on a single line.
| match callee with | ||
| | Direct _callee -> Cvar place_within_node | ||
| | Direct _callee -> | ||
| if Config.spacetime_call_counts then begin |
|
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. |
|
@dra27 I've merged your patch for Windows, thanks. |
| } allocation_point; | ||
|
|
||
| typedef struct { | ||
| value callee_node; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
Should this be protected by WITH_SPACETIME_CALL_COSTS?
There was a problem hiding this comment.
I think this should probably stay unguarded, since there is a named struct field at this point.
This patch provides for the counting of:
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:
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.