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

rework Instrument API to allow efficient implementation #257

Closed
njsmith opened this issue Jul 30, 2017 · 2 comments
Closed

rework Instrument API to allow efficient implementation #257

njsmith opened this issue Jul 30, 2017 · 2 comments

Comments

@njsmith
Copy link
Member

njsmith commented Jul 30, 2017

The Instrument API potentially adds overhead, since we have to call the hooks at all kinds of low level places that might be happening in tight loops. I'm not really worried about the overhead right now, but we should make sure that if we do decide to optimize it in the future, that we won't have to break the API to do so.

I think there are two parts to this:

  • Instead of blindly letting users muck around with the list of instruments, provide some explicit API for mutations. This is crucial because it lets us preprocess the list.

  • Make sure that we can tell by examining an Instrument object which hooks it actually provides. This way our preprocessing can tell things like "there are no Instruments registered for this event, don't even bother looking".

@njsmith
Copy link
Member Author

njsmith commented Aug 22, 2017

Regarding introspecting the instrument object to tell which hooks it provides: I think it's sufficient to detect either (a) the method is undefined, or (b) the method is inherited from the do-nothing trio.abc.Instrument class. These are both easy to detect and should cover all the cases that are likely to come up. We might need to note that like you shouldn't define a do-nothing method for no reason, but I doubt many people will need that advice.

@njsmith
Copy link
Member Author

njsmith commented Sep 26, 2017

After we kill current_instruments, think about this: #47 (comment)

@njsmith njsmith closed this as completed Dec 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant