Skip to content

Conversation

@duke8253
Copy link
Contributor

@duke8253 duke8253 commented Mar 31, 2020

As mentioned in #6577, we're now removing TSContSchedule, then renaming TSContScheduleOnPool to TSContSchedule. Also updated the docs and tests to reflect the change.

@duke8253 duke8253 added this to the 10.0.0 milestone Mar 31, 2020
@duke8253 duke8253 self-assigned this Mar 31, 2020
@duke8253 duke8253 force-pushed the master-rm_tscontschedule branch from 870cffb to b4d893e Compare March 31, 2020 20:06
Pool Properties
=========================== =======================================================================================
``TS_THREAD_POOL_NET`` Transaction processing threads. Continuations on these threads must not block.
``TS_THREAD_POOL_TASK`` Background threads. Continuations can perform blocking operations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO_BLOCK and BLOCK as aliases or instead of NET and TASK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a bad idea, @SolidWallOfCode what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit too radical of a change, given all the documentation and code that uses those terms.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding aliases would not require changing any of that.

required to have a mutex, which is provided to :func:`TSContCreate`.
Schedules :arg:`contp` to run :arg:`every` milliseconds, on a random thread that belongs to
:arg:`tp`. The :arg:`every` is an approximation, meaning it will be at least :arg:`every`
milliseconds but possibly more. Resolutions finer than roughly 5 milliseconds will not be
Copy link
Contributor

@ywkaras ywkaras Apr 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm is the effective resolution really that consistent across different flavors of Linux, and FreeBSD and macOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assume so since this is in the docs way back before I ever touched anything.

@bryancall
Copy link
Contributor

@duke8253 After the change @sudheerv made we are now scheduling the event on the same thread. This change makes it so that we now would schedule on the net thread pool vs the same thread? If this is the case, it would be better to schedule on the same thread instead of switching threads.

@duke8253
Copy link
Contributor Author

duke8253 commented Apr 3, 2020

@duke8253 After the change @sudheerv made we are now scheduling the event on the same thread. This change makes it so that we now would schedule on the net thread pool vs the same thread? If this is the case, it would be better to schedule on the same thread instead of switching threads.

Not exactly. It will schedule on a thread that matches the type, so the person scheduling must provide a type. If the type is the same as the current thread it's on, then it'll schedule on the current thread if there is no thread affinity set.

@SolidWallOfCode
Copy link
Member

There is an API function to schedule on a specific thread (which I think @keesspoelstra asked for at one summit, and lead to all of this...). Therefore, if what you want it so schedule on the current thread, you can easily do that.

@SolidWallOfCode
Copy link
Member

I'm impressed by the amount of documentation. Nice job!

Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should rename TSContScheduleOnPool() to be TSContSchedule(). The function requires a pool and it makes more sense to have the name TSContScheduleOnPool(). I have mix feeling on getting rid of TSContSchedule()

@duke8253
Copy link
Contributor Author

I'm fine with either, I'll revert back to not changing the names then?

@sudheerv
Copy link
Contributor

sudheerv commented Apr 20, 2020

I don't think we should rename TSContScheduleOnPool() to be TSContSchedule(). The function requires a pool and it makes more sense to have the name TSContScheduleOnPool(). I have mix feeling on getting rid of TSContSchedule()

Yeah, I tend to agree with Bryan - the naming of TSContScheduleOnPool() and TSContschedule() are more readable and intuitive to relate to the intended behavior. Even though TSContScheduleOnPool() can indirectly achieve the effect of scheduling on the current thread, it feels a bit too contrived and perhaps a bit needlessly complicated that it requires the user to explicitly pass a thread type, and it’d only pick the current thread when the affinity is not set. As a API user, one would much rather have a simple and direct API that says TSContSchedule() that defaults to the current thread without needing to worry about thread types. This is actually why I didn’t like reusing TSContScheduleOnPool() naming if we wanted to kill one of the API.

I wish we had the option of using default parameters, with which we probably could have condensed into one API much more intuitively. Short of that, I dont particularly think having two separate (clearly named) API in this case is confusing or overload. Rather, repurposing one API to support too many combinations seems the more confusing option.

Long story short, while I’m not -1 to condense into a single API and would be okay if everyone else feels that’s the right way to go, I’m definitely +1 on leaving the (2 separate) API as they are in ATS9 (current master).

@SolidWallOfCode
Copy link
Member

Given what I suggested a month ago I am glad to see @sudheerv and @bryancall have come around to my point of view. We should have TSContScheduleOnPool for pool based scheduling and TSContSchedulOnThread for thread based scheduling. Scheduling on the current thread is then

TSContScheduleOnThread(cont, TSEventThreadSelf());

Which I find much clearer than just TSContSchedule, which I think should be reserved for purely "automatic" scheduling, in situations where the continuation came from somewhere else and it should be sent back there (via thread affinity).

@duke8253 duke8253 force-pushed the master-rm_tscontschedule branch from 85c0725 to e00265b Compare April 27, 2020 17:59
@duke8253
Copy link
Contributor Author

Everything should be fixed by now.

@bryancall bryancall merged commit cefe482 into apache:master Apr 28, 2020
@zwoop zwoop modified the milestones: 10.0.0, 9.1.0 Jan 28, 2021
@zwoop
Copy link
Contributor

zwoop commented Jan 28, 2021

Reverted from the 9.1.x branch, and restored to Milestone 10.0.0 for now.

@zwoop zwoop modified the milestones: 9.1.0, 10.0.0 Jan 28, 2021
@zwoop zwoop modified the milestones: 10.0.0, 9.1.0 Feb 3, 2021
@zwoop zwoop modified the milestones: 10.0.0, 10-Dev Sep 9, 2021
@zwoop zwoop modified the milestones: 10-Dev, 10.0.0 Feb 2, 2023
@bryancall bryancall mentioned this pull request Aug 14, 2024
91 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants