Skip to content

Add new API TSContScheduleOnEntirePool and TSContScheduleEveryOnEntirePool #10742

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.

Sign up for GitHub

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

Jump to bottom
Merged
duke8253 merged 1 commit into from
Mar 4, 2024
Merged

Add new API TSContScheduleOnEntirePool and TSContScheduleEveryOnEntirePool #10742

duke8253 merged 1 commit into from
Mar 4, 2024

Conversation

@duke8253
Copy link
Contributor

@duke8253 duke8253 commented Nov 7, 2023 •
edited by bryancall
Loading

Adds two new APIs that allow scheduling continuations on all threads of a certain thread type (e.g. ET_NET, ET_TASK):

std::vector<TSAction> TSContScheduleOnEntirePool(TSCont contp, TSHRTime timeout, TSThreadPool tp)
std::vector<TSAction> TSContScheduleEveryOnEntirePool(TSCont contp, TSHRTime every, TSThreadPool tp)

The behavior is similar to EventProcessor::schedule_spawn().

@duke8253 duke8253 added this to the 10.0.0 milestone Nov 7, 2023
@duke8253 duke8253 self-assigned this Nov 7, 2023
@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch 2 times, most recently from d7b46aa to 6ce4110 Compare November 7, 2023 19:47
@bneradt
Copy link
Contributor

bneradt commented Nov 7, 2023

[approve ci]

1 similar comment
@duke8253
Copy link
Contributor Author

duke8253 commented Nov 7, 2023

[approve ci]

@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch 2 times, most recently from e682d6f to b1eee60 Compare November 8, 2023 15:51
@duke8253 duke8253 marked this pull request as draft November 8, 2023 16:13
@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch 5 times, most recently from bc44306 to 81ff8b3 Compare November 9, 2023 16:34
@duke8253 duke8253 marked this pull request as ready for review November 9, 2023 16:35
@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch from 81ff8b3 to 9336092 Compare November 9, 2023 16:54
SolidWallOfCode
SolidWallOfCode reviewed Nov 9, 2023
View reviewed changes
src/api/InkAPI.cc Outdated
}

std::vector<tsapi::c::TSAction>
tsapi::c::TSContScheduleOnEntirePool(TSCont contp, TSHRTime timeout, TSThreadPool tp)
Copy link
Member

@SolidWallOfCode SolidWallOfCode Nov 9, 2023

Choose a reason for hiding this comment

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

How does something in the C API return a std::vector?

Copy link
Contributor Author

@duke8253 duke8253 Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@bneradt said it's all c++ api now. Or are you talking about the tsapi::c part?

Copy link
Member

@SolidWallOfCode SolidWallOfCode Nov 9, 2023

Choose a reason for hiding this comment

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

The tsapi::c part.

SolidWallOfCode
SolidWallOfCode reviewed Nov 9, 2023
View reviewed changes
Copy link
Member

@SolidWallOfCode SolidWallOfCode 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 like the double vector implementation. I think it would be reasonable to have the core methods return TSAction instead of TSEvent, because

  • The latter can be extracted from the former.
  • It avoids the double allocation.
  • It hides the pointer hacking in the Event system instead of the API implementation.

@duke8253
Copy link
Contributor Author

duke8253 commented Nov 9, 2023

I don't like the double vector implementation. I think it would be reasonable to have the core methods return TSAction instead of TSEvent, because

  • The latter can be extracted from the former.

  • It avoids the double allocation.

  • It hides the pointer hacking in the Event system instead of the API implementation.

That does sound better, I'll change that. Did it this way since the other ones had that part in the API.

@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch from 9336092 to c91ba83 Compare November 9, 2023 22:27
SolidWallOfCode
SolidWallOfCode previously approved these changes Nov 9, 2023
View reviewed changes
zwoop
zwoop reviewed Nov 10, 2023
View reviewed changes
doc/developer-guide/api/functions/TSContScheduleEveryOnEntirePool.en.rst Outdated
is effective until the continuation :arg:`contp` is being dispatched. However, if it is
scheduled on another thread this can be problematic to be correctly timed. The return value
can be checked with :func:`TSActionDone` to see if the continuation ran before the return,
which is possible if :arg:`timeout` is `0`.
Copy link
Contributor

@zwoop zwoop Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You refer to this as "timeout" here in the doc, but in the prototype it's "every". That seems inconsistent. Maybe it should be "interval" ?

Also, since you added an explicit "call once" API, what does a "every" of 0 mean ?? Should we even allow that ?

Copy link
Contributor Author

@duke8253 duke8253 Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I copied some of the text from the other docs , every == 0 is not allowed for any of the TSContScheduleEveryXXX calls. I'll fix this.

zwoop
zwoop requested changes Nov 10, 2023
View reviewed changes
doc/developer-guide/api/functions/TSContScheduleOnEntirePool.en.rst
#include <ts/ts.h>
.. function:: std::vector<TSAction> TSContScheduleOnEntirePool(TSCont contp, TSHRTime timeout, TSThreadPool tp)
Copy link
Contributor

@zwoop zwoop Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Timeout seems wrong name here. It's a delay, right ? I got confused here because the "every" version also uses both timeout and delay "randomly"?

Are these API prototypes the same as what was sent to the mailing list? If not, maybe an update there as well ?

Copy link
Contributor Author

@duke8253 duke8253 Nov 10, 2023

Choose a reason for hiding this comment

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

I'm just following the naming convention for the other TSContSchedule APIs. I'll update the email in the mailing list to show the changes of this PR.

The timeout is kind of like a delay, but it could also be a negative number for negative queue events.

Copy link
Member

@SolidWallOfCode SolidWallOfCode Nov 17, 2023

Choose a reason for hiding this comment

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

I thought we got rid of the negative queue.

Copy link
Contributor Author

@duke8253 duke8253 Nov 17, 2023

Choose a reason for hiding this comment

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

pretty sure it's still here?

trafficserver/src/iocore/eventsystem/UnixEThread.cc

Line 241 in 0d14262

process_queue(&NegativeQueue, &ev_count, &nq_count);

@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch 2 times, most recently from 9683305 to 291ee15 Compare November 10, 2023 23:18
@apache apache deleted a comment from duke8253 Nov 15, 2023
@duke8253
Copy link
Contributor Author

duke8253 commented Dec 1, 2023

@zwoop I think this is ready for another review

@bneradt
Copy link
Contributor

bneradt commented Dec 1, 2023

[approve ci format]

@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch from 291ee15 to d277f6a Compare December 1, 2023 22:09
@bryancall bryancall requested a review from zwoop February 5, 2024 23:47
zwoop
zwoop previously approved these changes Feb 6, 2024
View reviewed changes
@duke8253 duke8253 modified the milestones: 10.0.0, 10.1.0 Mar 1, 2024
@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch from d277f6a to c527966 Compare March 1, 2024 16:39
@duke8253 duke8253 force-pushed the master-schedule_on_entire_pool branch from c527966 to 6c5e1e0 Compare March 1, 2024 16:52
@duke8253
Copy link
Contributor Author

duke8253 commented Mar 1, 2024

[approve ci autest]

@duke8253 duke8253 requested a review from zwoop March 4, 2024 16:00
@duke8253 duke8253 merged commit 1125cc2 into apache:master Mar 4, 2024
@duke8253 duke8253 mentioned this pull request Aug 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zwoop zwoop zwoop approved these changes

+1 more reviewer

@SolidWallOfCode SolidWallOfCode SolidWallOfCode left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@duke8253 duke8253

Labels

Event System New Feature Release Doc Done TS API

Projects

None yet

Milestone

10.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@duke8253 @bneradt @zwoop @SolidWallOfCode @cmcfarlen