Page MenuHomePhabricator

Delete ActionManager and replace it with a simple callback on the Context

Authored by mehdi_amini on Feb 25 2023, 10:18 PM.



The concept of the ActionManager acts as a sort of "Hub" that can receive
various types of action and dispatch them to a set of registered handlers.
One handler will handle the action or it'll cascade to other handlers.

This model does not really fit the current evolution of the Action tracing
and debugging: we can't foresee a good case where this behavior compose with
the use-case behind the handlers. Instead we simplify it with a single
callback installed on the Context.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 25 2023, 10:18 PM
mehdi_amini requested review of this revision.Feb 25 2023, 10:18 PM
rriddle accepted this revision.Feb 27 2023, 9:42 AM
rriddle added inline comments.

The naming here feels much too general for something specific to actions, I'd rather have this be an explicitly named API.

Also, why "dispatch"? instead of something like "execute" or something more aligned with how actions define the verbage.

This revision is now accepted and ready to land.Feb 27 2023, 9:42 AM
mehdi_amini added inline comments.Feb 27 2023, 10:54 AM

I am fine with execute, I went back and forth on it and ended up using dispatch() to encourage feedback :)

What made me hesitate is that an Action can be skipped (by debug counters for example).

rriddle added inline comments.Feb 27 2023, 10:55 AM

I'd be fine with dispatch, but really just want it to be consistent. I think we use execute in the action verbiage (unless that got fixed?). Mainly just want it to be clear what the API is for when reading it, and be able to attribute it to actions.

mehdi_amini added inline comments.Feb 27 2023, 11:00 AM

Could use delegate() as well (dispatch may feel like we may use a thread pool or something like that).

You also said it is too generic, do you mean you would prefer to see something like context.delegateAction() ?

The call sites will look like context.execute<PassAction>() right now, that would look like context.executeAction<PassAction>() instead.

I haven't written the doc for all this, I'll add a revision when we're done with all the revisions (so that I document the final state).

rriddle added inline comments.Feb 27 2023, 11:04 AM

Yeah, I'm a bit concerned about the API being very generically named. I was less concerned about the (templated) callsites, and more with users reading API (needs a lot of context to see what's going on). Maybe this concern is overblown. Can you wrap the API in a comment block? Might help with grouping/reading:

// Action API

/// Dispatch the provided action to the handler if any, or just execute it.
void dispatch(function_ref<void()> actionFn, const tracing::Action &action) {
  if (LLVM_UNLIKELY(hasActionHandler()))
    dispatchInternal(actionFn, action);
jpienaar added inline comments.Feb 27 2023, 8:53 PM

No I think that's valid concern, I'd even suggest consisting affixing (ActionHandlerTy, executeAction)

Rename the API as executeAction()

mehdi_amini marked 5 inline comments as done.Mar 6 2023, 7:27 AM
This revision was landed with ongoing or failed builds.Mar 6 2023, 11:26 PM
This revision was automatically updated to reflect the committed changes.