Skip to content

Conversation

@timpalpant
Copy link
Contributor

This allows the user to register another thread for real time profiling by passing its identity (thread id), rather than needing to call insert_real_time_thread() from within each thread.

Fixes #201. Depends on / includes #204 to fix the real_time_threaded test.

@arigo
Copy link
Contributor

arigo commented Aug 10, 2019

The PyArg_ParseTuple() need some tweaking:

  • You must initialize the local thread_id variable you pass as optional argument, or make that argument not optional, because right now if we call the function with no argument then we get an undefined value in the local thread_id variable.

  • Using the argument 'k' is arbitrary, as we don't know the size of pthread_t is the same as the size expected by 'k'. You need to pass a different variable of the correct type for 'k', and then cast that into a pthread_t, which ideally should be done carefully (see CPython for how to carefully cast between integers and pthread_t variables)

@timpalpant timpalpant force-pushed the insert-thread-by-id branch from 56b00b1 to bf1e2db Compare August 10, 2019 17:23
@timpalpant
Copy link
Contributor Author

Thanks @arigo, I appreciate it. I've amended the change to try to address these issues. The first is an obvious blunder, and I've updated to initialize the variable.

The second is a bit more intricate as you note. I've updated to safely cast to pthread_t in the same manner as PyThread_start_new_thread, taking into account the size of pthread_t. (Interestingly it seems to be a plain cast in PyThread_get_thread_ident below).

Apologies if this is different than what you had in mind. I also consulted with the standard library implementation of signal.pthread_kill(thread_id) that has a similar interface, but it seems to do a naive cast from unsigned long to pthread_t.

@arigo
Copy link
Contributor

arigo commented Aug 11, 2019

Looks good to me. I'm not really contributing to vmprof, but I don't know if anyone is at the moment, so I'll go ahead and merge your pull request. Sorry if I'm stepping on someone else's toes.

@arigo arigo merged commit 45eb346 into vmprof:master Aug 11, 2019
@bawr
Copy link
Contributor

bawr commented Aug 27, 2019

Thanks @timpalpant - this actually makes real-time profiling more useful for my use case, too, and I appreciate your in-depth diagnosis of EINTR being the original culprit for tests failing. Out of curiosity - do you happen to be using the real-time stuff as well, or was it a more preemptive fix? (I'm trying to gauge interest because actually documenting the real-time capabilities was on my list since forever, but I vaguely assumed this is a niche application, so I never got around to it.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal] Allow inserting a different thread by id with insert_real_time_thread

3 participants