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

IomgrEventEngine #29616

Merged
merged 29 commits into from
May 13, 2022
Merged

IomgrEventEngine #29616

merged 29 commits into from
May 13, 2022

Conversation

drfloob
Copy link
Member

@drfloob drfloob commented May 9, 2022

@ctiller

Timer-only implementation. Mutates, tracks, and shuttles EventEngine API calls to iomgr.

Performance caveats:

  • for Cancel to return true/false, timers are registered in a hash set, which is guarded by a lock. init/cancel/run all require acquiring the lock.
  • every timer init does a small heap allocation.

@drfloob drfloob added the release notes: no Indicates if PR should not be in release notes label May 9, 2022
@ctiller
Copy link
Member

ctiller commented May 9, 2022

I haven't done a super close review, but the basics look like a good start.

Going forward, we should fork the grpc iomgr timer code into iomgr_eventengine.
This will ultimately allow us to eliminate the hash table (and global mutex!) and the allocation.

Playbook:

  1. Fork grpc iomgr code
  2. The two uintptr_t's in TaskHandle become: a pointer to the (forked/renamed) grpc_timer object, and a sequence number
  3. grpc_timer additionally adds a sequence number for the TaskHandle that's currently active
  4. When allocating a timer object, we first check a free list, if it's empty we take an allocation of a new object; if the free list is not empty we take one object off the free list
  5. When freeing a timer object we add it to a free list (this can be done entirely with atomics - for an example look in core_configuration.cc for the registration list) -- note that this means that these timer objects are never deallocated
  6. When we need to 'look inside' a TaskHandle we compare sequence numbers of the handle and the timer object it points to, and iff they match we return the pointer, otherwise we do not (it's a stale pointer)

Now the majority of timers do not allocate, and there's no global choke point to synchronize around (if the free list becomes a contention point we can even shard that)

@drfloob
Copy link
Member Author

drfloob commented May 9, 2022

Going forward, we should fork the grpc iomgr timer code into iomgr_eventengine.

That's the plan. This is very much a temporary thing that can unblock other EventEngine progress.

Now the majority of timers do not allocate, and there's no global choke point to synchronize around (if the free list becomes a contention point we can even shard that)

That's an excellent trick. I think I could do something like that using the current {ptr, aba_token} in this implementation too, but doing the iomgr code migration first makes more sense.

@drfloob drfloob marked this pull request as ready for review May 9, 2022 23:08
It previously assumed single-threaded operations. Logging on EventEngine
init can ask for the time while this test is changing it.
It was possible for an immediate callback to return before its handle
was added to the known handles set.
@drfloob drfloob changed the title [EXPERIMENT] IomgrEventEngine IomgrEventEngine May 11, 2022
@drfloob drfloob requested a review from ctiller May 11, 2022 22:18
@drfloob
Copy link
Member Author

drfloob commented May 11, 2022

I believe this is ready for review.

Copy link
Member

@ctiller ctiller left a comment

Choose a reason for hiding this comment

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

This is a reasonable first step, and we know it's not a final design.
We know the follow-up actions to take, so let's keep taking those monotonic steps.

@drfloob
Copy link
Member Author

drfloob commented May 13, 2022

The 5 failed tests are all unrelated known issues: XDS, timeouts, or infra flakes.

@drfloob drfloob merged commit 7f09b98 into grpc:master May 13, 2022
drfloob added a commit that referenced this pull request May 13, 2022
ctiller pushed a commit that referenced this pull request May 13, 2022
drfloob added a commit that referenced this pull request May 13, 2022
drfloob added a commit that referenced this pull request May 16, 2022
* Revert "Revert "IomgrEventEngine (#29616)" (#29692)"

This reverts commit 246d13e.

* temporarily disable EE usage to coordinate landing

* spelling
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 16, 2022
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request May 24, 2022
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request May 24, 2022
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request May 24, 2022
* Revert "Revert "IomgrEventEngine (grpc#29616)" (grpc#29692)"

This reverts commit 246d13e.

* temporarily disable EE usage to coordinate landing

* spelling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/medium imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants