Port eventlog to CTF #527

abbysmal · 2021-03-31T13:19:45Z

Still some work to do on this one.

Some bits to refactor:

I think the core logic is fine, but I may need to stress test it.
I need to make sure we get similar logs between the previous (parallel_minor_gc) eventlog and this one.
I need to push changes to https://github.com/ocaml-multicore/eventlog-tools

I think I may be missing some events that existed in our previous eventlog implementation, I need to check that.

runtime/eventlog.c

runtime/domain.c

ctk21 · 2021-04-21T08:07:14Z

Minor nit, but can we clean up the instrumented_runtime changes to configure.ac?

Right now vs mainline OCaml we have the following confusing diff in configure.ac:

@@ -61,7 +61,7 @@ toolchain="cc"
 profinfo=false
 profinfo_width=0
 extralibs=
-instrumented_runtime=true
+instrumented_runtime=false
 instrumented_runtime_ldlibs=""

 # Information about the package
@@ -227,12 +227,11 @@ AC_ARG_ENABLE([dependency-generation],
 AC_ARG_VAR([DLLIBS],
   [which libraries to use (in addition to -ldl) to load dynamic libs])

-# Disable instrumented-runtime with multicore
 AC_ARG_ENABLE([instrumented-runtime],
   [AS_HELP_STRING([--enable-instrumented-runtime],
     [build the instrumented runtime @<:@default=auto@:>@])],
   [],
-  [enable_instrumented_runtime=no])
+  [enable_instrumented_runtime=auto])

 AC_ARG_ENABLE([vmthreads], [],
   [AC_MSG_ERROR([The vmthreads library is no longer available. \

I'm presuming we can revert the changes in the block with the # Disable instrumented-runtime with multicore?

Also it might be nice to comment the line where we have enabled the instrumented_runtime in multicore, saying that we have the instrumented runtime always enabled by default.

ctk21 · 2021-04-21T08:21:30Z

Another thing from a user perspective, do we have instructions on how to switch on and generate a trace using CTF which can be viewed by a user?
(presumably switch on is still OCAMLRUNPARAM=e as that hasn't been changed in startup_aux.c?)
It's possible the instructions belong in the Wiki and not the PR, but we should try to get that lined up at the point of merge for the PR.

…nter in Multicore.

sadiqj · 2021-04-30T14:24:12Z

runtime/caml/domain_state.tbl

-DOMAIN_STATE(uintnat, eventlog_startup_timestamp)
-DOMAIN_STATE(long, eventlog_startup_pid)
+/*****************************************************************************/
+/* Detailed stats */


How have these reappeared? They were removed in #535

sadiqj · 2021-04-30T14:26:43Z

runtime/eventlog.c

-static struct event_buffer* evbuf;
+#define evbuf Caml_state->eventlog_buffer
+
+static int64_t startup_timestamp;


Is this better as time_t?

sadiqj · 2021-04-30T14:33:02Z

runtime/eventlog.c

-    if (*toggle == 'p')
-      Caml_state->eventlog_paused = 1;
+    eventlog_enabled = 1;
+    // FIXME(engil): disabling eventlog pause on Multicore for now


This probably needs to be fixed?

sadiqj · 2021-04-30T14:33:54Z

runtime/eventlog.c

  }
 }

 /*  Note that this function does not trigger an actual disk flush, it just
    pushes events in the event buffer.
 */
+// FIXME(engil): alloc events are not flushed on Multicore for now.


Is this still an issue?

sadiqj · 2021-04-30T14:34:14Z

runtime/eventlog.c

@@ -358,26 +409,29 @@ void caml_ev_flush()

 void caml_eventlog_disable()
 {
-  Caml_state->eventlog_enabled = 0;
-  teardown_eventlog();
+  // FIXME(engil): disabling runtime eventlog disable on Multicore for now


Likewise, this needs to be fixed.

sadiqj · 2021-04-30T14:34:59Z

runtime/eventlog.c

-  CAMLassert(v == Val_unit);
-  if (Caml_state->eventlog_enabled)
-    Caml_state->eventlog_paused = 0;
+  // FIXME(engil): disabling eventlog pause on Multicore for now


sadiqj · 2021-04-30T14:35:36Z

runtime/eventlog.c

-    if (evbuf && Caml_state->eventlog_out)
-      flush_events(Caml_state->eventlog_out, evbuf);
-  };
+  // FIXME(engil): disabling eventlog pause on Multicore for now


And this one.

sadiqj · 2021-04-30T14:37:37Z

I think we need to address these FIXMEs before merging this. I'm happy to see we've eliminated all but one of the thread locals (and the remaining one makes sense).

abbysmal · 2021-05-04T14:23:52Z

Thanks for your review. I addressed the outstanding issues.

I implemented pausing and resume in a way that looks alright.
I introduced the eventlog_paused global variable so any domain requesting a pause effectively interrupt the eventlog machinery.
We could also introduce it in a domain local way but I think the former is a more common usecase than the later.
eventlog_teardown will take care of flushing remaining event on domain termination even if paused, so there's no need for the explicit flush from trunk.
Sorry for the detailed stats, this was a rebase mishap.

There is one FIXME that I did turn into a TODO, and it's related to the alloc counter events.
It is a metric that was found in trunk, and not on our implementation of eventlog. Considering how different the allocators are, I'm not sure how and where we want them.
I think it is fine to be left as a TODO item, though I did made sure the mechanism is now working and flushed correctly if we ever put them in use again.

sadiqj · 2021-05-06T14:41:05Z

This addresses me earlier concerns. Thanks @Engil

…2+domains+effects+ctf Port eventlog to CTF

sadiqj self-requested a review April 7, 2021 16:35

sadiqj reviewed Apr 7, 2021

View reviewed changes

runtime/eventlog.c Outdated Show resolved Hide resolved

sadiqj reviewed Apr 7, 2021

View reviewed changes

runtime/eventlog.c Outdated Show resolved Hide resolved

ctk21 reviewed Apr 9, 2021

View reviewed changes

runtime/domain.c Outdated Show resolved Hide resolved

abbysmal force-pushed the 4.12+domains+effects+ctf branch 3 times, most recently from fb6156d to 90763a9 Compare April 13, 2021 12:32

abbysmal added 5 commits April 28, 2021 12:45

configure: enable instrumented_runtime

78463a7

eventlog: first pass

fcf37fe

testsuite: disable instrumented runtime test for now

285c259

runtime: small adjustment to OCAML_EVENTLOG_PREFIX

01f2295

runtime: caml_ev_alloc_flush is not used for now, there's no such cou…

37b7f73

…nter in Multicore.

abbysmal force-pushed the 4.12+domains+effects+ctf branch from d5b5c3e to 37b7f73 Compare April 28, 2021 10:45

kayceesrk mentioned this pull request Apr 30, 2021

Reimplement broken pausetimes support ocaml-bench/sandmark#218

Closed

sadiqj suggested changes Apr 30, 2021

View reviewed changes

abbysmal added 4 commits May 4, 2021 13:51

runtime: startup_timestamp as time_t

8d4e6fb

runtime: bring back paused handling in eventlog

1e79541

runtime: add back alloc counters handling in eventlog

fad4291

fix rebase mishap

27ba90e

sadiqj approved these changes May 6, 2021

View reviewed changes

ctk21 merged commit b23a416 into ocaml-multicore:4.12+domains+effects May 10, 2021

sadiqj pushed a commit to sadiqj/ocaml that referenced this pull request Jan 10, 2022

Merge pull request ocaml-multicore/ocaml-multicore#527 from Engil/4.1…

dbaea4d

…2+domains+effects+ctf Port eventlog to CTF

c-cube pushed a commit to c-cube/ocaml that referenced this pull request Feb 3, 2022

Merge pull request ocaml-multicore/ocaml-multicore#527 from Engil/4.1…

4668501

…2+domains+effects+ctf Port eventlog to CTF

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port eventlog to CTF #527

Port eventlog to CTF #527

abbysmal commented Mar 31, 2021

ctk21 commented Apr 21, 2021

ctk21 commented Apr 21, 2021

sadiqj Apr 30, 2021

sadiqj Apr 30, 2021

sadiqj Apr 30, 2021

sadiqj Apr 30, 2021

sadiqj Apr 30, 2021

sadiqj Apr 30, 2021

sadiqj Apr 30, 2021

sadiqj commented Apr 30, 2021

abbysmal commented May 4, 2021 •

edited

Loading

sadiqj commented May 6, 2021

Port eventlog to CTF #527

Port eventlog to CTF #527

Conversation

abbysmal commented Mar 31, 2021

ctk21 commented Apr 21, 2021

ctk21 commented Apr 21, 2021

sadiqj Apr 30, 2021

Choose a reason for hiding this comment

sadiqj Apr 30, 2021

Choose a reason for hiding this comment

sadiqj Apr 30, 2021

Choose a reason for hiding this comment

sadiqj Apr 30, 2021

Choose a reason for hiding this comment

sadiqj Apr 30, 2021

Choose a reason for hiding this comment

sadiqj Apr 30, 2021

Choose a reason for hiding this comment

sadiqj Apr 30, 2021

Choose a reason for hiding this comment

sadiqj commented Apr 30, 2021

abbysmal commented May 4, 2021 • edited Loading

sadiqj commented May 6, 2021

abbysmal commented May 4, 2021 •

edited

Loading