Skip to content

Replace LogicalThread with WorkSerializer#21846

Merged
yashykt merged 4 commits intogrpc:masterfrom
yashykt:workserializer
Feb 4, 2020
Merged

Replace LogicalThread with WorkSerializer#21846
yashykt merged 4 commits intogrpc:masterfrom
yashykt:workserializer

Conversation

@yashykt
Copy link
Member

@yashykt yashykt commented Jan 30, 2020

  1. Renamed LogicalThread with WorkSerializer
  2. Removed dependency on RefCounted class for RefCountedPtr
  3. DebugLocation dependency still exists because we can't use absl::SourceLocation yet.

(Forked from #21361)


This change is Reviewable

@yashykt yashykt added the release notes: no Indicates if PR should not be in release notes label Jan 30, 2020
@yashykt yashykt marked this pull request as ready for review January 30, 2020 21:48
@yashykt yashykt requested a review from markdroth January 30, 2020 21:48
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks really good! All my comments are minor.

Please let me know if you have any questions about any of this.

Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @yashykt)


src/core/lib/iomgr/work_serializer.h, line 35 at r1 (raw file):

namespace grpc_core {
extern DebugOnlyTraceFlag grpc_work_serializer_trace;

Is this needed anywhere outside of the .cc file? If not, let's remove it from here.


src/core/lib/iomgr/work_serializer.h, line 37 at r1 (raw file):

extern DebugOnlyTraceFlag grpc_work_serializer_trace;

class WorkSerializerImpl : public Orphanable {

Can we move this into the .cc file, so it's not in the header at all? If you run into problems whereby the OrphanablePtr<> dtor needs to know the definition of WorkSerializerImpl, you might be able to work around them by declaring a dtor for WorkSerializer and defining it in the .cc file (even if it's just an empty function).

If that won't work for some reason, let's at least make WorkSerializerImpl a private member of WorkSerializer.


src/core/lib/iomgr/work_serializer.h, line 55 at r1 (raw file):

// WorkSerializer is a mechanism to schedule callbacks in a synchronized manner.
// All callbacks scheduled on a WorkSerializer instance will be executed
// serially in a borrowed thread. The API provides a FIFO guarantee to the

I think this documentation should explicitly point out the following things:

  • The borrowed threads are the ones that call Run().
  • When you call Run(), your callback might run directly inline, or it might run in a different thread that is already inside of Run(). If your callback runs directly inline, other callbacks from other threads might also run inline before Run() returns.
  • Because an arbitrary set of callback might run directly inline, you should generally not hold any locks while calling Run().

src/core/lib/iomgr/work_serializer.h, line 59 at r1 (raw file):

class WorkSerializer {
 public:
  WorkSerializer() { impl_ = MakeOrphanable<WorkSerializerImpl>(); }

impl_ can be initialized in an initializer list instead of in the ctor body.


src/core/lib/iomgr/work_serializer.cc, line 105 at r1 (raw file):

      return;
    }
    // There is atleast one callback on the queue. Pop the callback from the

s/atleast/at least/

Copy link
Member Author

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @markdroth)


src/core/lib/iomgr/work_serializer.h, line 35 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Is this needed anywhere outside of the .cc file? If not, let's remove it from here.

Done.


src/core/lib/iomgr/work_serializer.h, line 37 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Can we move this into the .cc file, so it's not in the header at all? If you run into problems whereby the OrphanablePtr<> dtor needs to know the definition of WorkSerializerImpl, you might be able to work around them by declaring a dtor for WorkSerializer and defining it in the .cc file (even if it's just an empty function).

If that won't work for some reason, let's at least make WorkSerializerImpl a private member of WorkSerializer.

Ah! That's a smart way! Worked!


src/core/lib/iomgr/work_serializer.h, line 55 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think this documentation should explicitly point out the following things:

  • The borrowed threads are the ones that call Run().
  • When you call Run(), your callback might run directly inline, or it might run in a different thread that is already inside of Run(). If your callback runs directly inline, other callbacks from other threads might also run inline before Run() returns.
  • Because an arbitrary set of callback might run directly inline, you should generally not hold any locks while calling Run().

Done.


src/core/lib/iomgr/work_serializer.h, line 59 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

impl_ can be initialized in an initializer list instead of in the ctor body.

Done.


src/core/lib/iomgr/work_serializer.cc, line 105 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

s/atleast/at least/

Done.

Copy link
Member Author

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @markdroth)


src/core/lib/iomgr/work_serializer.h, line 37 at r1 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Ah! That's a smart way! Worked!

Done.

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Just one more small change needed here.

Thanks for doing this!

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yashykt)


src/core/lib/iomgr/work_serializer.h, line 36 at r2 (raw file):

namespace grpc_core {

class WorkSerializerImpl;

Please make this declaration a private member of WorkSerializer.

Copy link
Member Author

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

Reviewable status: 19 of 21 files reviewed, 1 unresolved discussion (waiting on @markdroth)


src/core/lib/iomgr/work_serializer.h, line 36 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please make this declaration a private member of WorkSerializer.

Done.

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Looks great!

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@yashykt
Copy link
Member Author

yashykt commented Feb 4, 2020

issues: #21221, #21895, #16003, #21906, #21907, #20703

@yashykt
Copy link
Member Author

yashykt commented Feb 4, 2020

Thanks for reviewing!

@yashykt yashykt merged commit c3bcb6c into grpc:master Feb 4, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
@yashykt yashykt deleted the workserializer branch May 18, 2023 20:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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.

3 participants