[RFC] Adding names to PackedFunc

Development pre-RFC
#1

I propose we add an associated name to every packed function in order to make the error messages clearer. Right now if you call a packed function with the wrong number of arguments, you get this error: Check failed: nargs == args.size() (3 vs. 2) : Expect 3 arguments but get 2. If we add a name to each packed function, we can create clearer error messages like TVMError: tir.LT expects 3 arguments, but 2 were provided.

There are a couple choices around adding names to packed functions:

Naming Convention

What should we call the name field of the packed function? Two possible options:

  • A0: name_hint: indicates that the name may not exist and may not be unique. Also matches the naming convention on other objects.
  • A1: name: simple, straight forward.

Field Type

How should we store the name?

  • B0: Use String as the field and add constructor that uses a default name if none is given.
  • B1: Use Optional<String> to avoid the overhead of having a String object on each member (saves one pointer and one uint64_t over String).
  • B2: Move PackedFunc to the object system and a subclassed version that has a name.

API

Do we treat the name as immutable or not?

  • C0: Name is immutable: simple, but may prevent us from setting names on c-callbacks. Also, will have to do mutation to set the name from TVM_REGISTER_GLOBAL.
  • C1: Name is mutable via setter method.
  • C2: Name is write once. If we use Optional<String>, we allow setting the name only if the optional is not set.

What are everyones thoughts on this?

@tqchen @zhiics @jroesch @junrushao

1 Like
#2

Just a note on B1. Optional<String> won’t safe the storage space(since we still need the pointer), but it will save the construction time(no need to construct a concrete string everytime)

#3

one thing i’m not sure how we should handle is PackedFuncs built out of closures. We do this a lot, and on the one hand while it helps with code readability, it makes tracing code from a backtrace more complex, since the backtrace library seems to list closures in as offsets of their parent function.

#4

I would definitely like to improve the quality of the backtraces, but I think it is orthogonal to adding names to the packed functions.

#5

Naming convention. I personally prefer A1, because it looks like the names are unique and we won’t mutate them very frequently if the packed functions are in the global registry - which means it is a concrete name, not a hint.

Field Type. I agree with Tianqi’s point that we need PackedFunc to be “with a goal to gain std::function level speed”, plus zero memory overhead. Therefore, I am not quite convinced with B0/B1/B2 yet. What about we do this (say B4):

  • leave PackedFunc as it is (unnamed closure), which has std::function level speed and memory footprint
  • introduce a TVM Object Callable, which has two fields
    • PackedFunc as its internally maintained type erased function
    • Optional<CalleeInfo> as its metadata, which includes: name of the function, filename and lineno of its definition site, etc

API. I think the difference between C0/C1/C2 are minor. I do not have strong opinion on the choice, although it seems that the name is a write-once field (C2). Making it mutable or immutable or somewhat mutable does not matter much though (IMO).

#6

@tkonolige yes, but we frequently e.g. from GetFunction return PackedFuncs that point to closures. we need to decide how to name those, and if we are going to require a name.

#7

I’m a little unclear on why the tiny overhead of adding a String makes significant change in performance. Are we repeatedly constructing and destroying millions of functions?

But if people are really concerned about the performance impact, I can put together a benchmark to see what the impact actually is.

#8

To be clear, I don’t think the overhead is a big deal. I am just talking from the angle that how well we can do

#9

The reason I’m bringing up the issue of naming PackedFunc closures is that most of our object member functions are closures returned from GetFunction. I disagree that improving the backtraces is entirely orthogonal, because the original motivation of this post was a bad error message:

Check failed: nargs == args.size() (3 vs. 2) : Expect 3 arguments but get 2

Ordinarily, you could check the backtrace to see which function is generating this error message, but because most PackedFunc are calling into C++ lambdas, the stacktrace tends to display the name of the function that instantiates the original PackedFunc. In a future where we autogenerate the GetFunction body, we could probably work around this by requiring all PackedFunc to call out to specific C++ functions, rather than closures which wrap functions of similar-but-not-quite-identical name and may do minimal processing or validation.

Do we have a strawman of how we might use the name to annotate the error messages? I’m concerned that the name is needed in unpack_call.

#10

In this situation, good backtraces would not fix the error message as it is in the generic dispatch function for all PackedFuncs. And this check occurs before we call into the contained function proper. However it should be pretty easy to add names to each of the returned PackedFuncs. If we want to force names on PackedFuncs, we can make this field mandatory, but that would be a pretty breaking change.

There’s PR here that implements the changes and fixes the error message above: https://github.com/apache/tvm/pull/7108.

#11

For the core runtime feature as this one, we should think of the best possible impl, ideally, we want to hold it up to the standard of standard stdc++ libraries.

In the particular case, I think having a Optional<String> that defaults to should be fine nullptr since it is one additional slot, and std::function already have quite some slot. Having additional allocation of a default string is less desirable for the anoymous case, since we can implement a GetName that returns the right <anoymous> name in the case of error reporting.

1 Like
#12

it seems like though, this is the only such case where an improved backtrace wouldn’t fix the error message–because the error is generated by a wrapping PackedFunc. Perhaps it makes sense to have the name live in the closure used by the wrapping PackedFunc?

#13

I am not sure if having one more unremovable slot inside a PackedFunc is perfect, especially when we want to hold up to stdc++ perf…Adding a wrapper object like Callable around could potentially provide more choices to developers.

#14

Given std::function already takes quite a lot of space (check it out :slight_smile: ), adding an additional slot(64bit) without allocating string may not be a big issue if we agree that name is important.

The Callable proposal is also OK, but will depend on moving PackedFunc to object itself.

I also believe @areusch 's proposal of (optionally) capturing the string as part of closure might be possible, although perhaps harder than the slot impl.

#15

i mostly would like to see us prove out the need for the slot impl beyond updating the error messages in TypedPackedFunc. the callee cannot typically assume it has access to the PackedFunc which called it.

to summarize my opinion:

  1. if we are just going to push everything to TypedPackedFunc, it seems like we should just make those errors better via closure, and avoid the additional explicit memory burden. it’s a much less invasive change and the use case is much more clear.
  2. if we are going to do something more complex such as raise specific error classes, catch them in the PackedFunc call, and mutate them to include the name of the PackedFunc before re-raising, then perhaps it makes sense to actually add an explicit name.
#16

I’m trying to improve two errors: Check failed: nargs == args.size() (3 vs. 2) : Expect 3 arguments but get 2 and Check failed: type_code_ == kTVMObjectHandle (11 vs. 8) : expected Object but get str. In these cases, having the name of the function is incredibly helpful. In these cases it may be possible to just capture the name in the closure of the TypedPackedFunc. I’ll look into it.

The reason I went for the field approach is that having the function name available could improve error messages in other places. For example, we could now show the name of the function in python when working with an interactive session.

#17

Okay. Then I dont oppose the idea of having an additional slot…

#18

@areusch Here is an alternative approach to adding a name to the TypedPackedFunc: https://github.com/apache/tvm/pull/7152. It captures a name into the lambda instead. Seems to work fine for generating the error messages I listed above.

#19

Yeah. Given we all agree with adding the name and the way to add the name, we should move fast to reach the final conclusion.

I am thinking if we can generalize the slot from a StringObj to a TVM object that contains other information, including line number and file name:

class PackedFuncInfoObj : public tvm::runtime::Object {
 public:
  String name;
  String filename;
  int lineno;
};

class PackedFuncInfo;

In the packed function, we could have:

Optional<runtime::PackedFuncInfo> info;

With this object, we can add more information about the packed function for easy debugging. @tkonolige what do you think?

#20

okay, my previous concerns about adding a new slot were not entirely founded because I had forgotten in the c++ runtime that we maintain a refcounted PackedFunc object when a PackedFunc is returned to the frontend. i’m okay with either approach, but usually it’s best to avoid general-purpose descriptive data structures without a clear use. I think that in this case, the use case presented so far is confined to just the TypedPackedFunc argument binding code, so I sort of prefer not to add more global data structures without > 1 consumer.

I imagine it would also be useful to be able to lookup a PackedFunc’s name too, so long as code didn’t start inspecting PackedFunc names to break the callback abstraction (this would be a net loss in the TVM design, IMO). For example, we should not encourage this kind of thing:

class GenericPass : Pass {
  GenericPass(TypedPackedFunc<bool, Node*> do_processing) : do_processing_{do_processing} {}

  virtual void VisitSomeNodeSubclass(Node* n) {
    if (do_processing_.name == "the_normal_thing") {
      do_processing_(n.child);
    } else {
      do_processing_(n);
    }
  }
1 Like