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

DNS: threaded resolver [WIP] #2829

Closed

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Jun 14, 2016

This Pull Request proposes to call getaddrinfo within threads and to synchronize with the main thread to receive the request and send back the response, thus allowing to not stop-the-world whenever a Fiber needs to resolve a Domain Name. The threadpool size can be adjusted with Socket::Addrinfo.threadpool_size = 4 for example before a Domain Name is first resolved, or even disabled with Socket::Addrinfo.threadpool_size = 0.

  • start configurable threadpool;
  • call getaddrinfo in threads;
  • fallback to call getaddrinfo from the main thread if the threadpool is disabled (ie. to set 0);
  • modify IPSocket, TCPSocket, TCPServer and UDPSocket to use DNS.getaddrinfo;
  • drop IPSocket.getaddrinfo or and avoid a DNS namespace to use Socket::Addrinfo instead;
  • consider reducing duplication of DNS::Addrinfo with Socket::IPAddress if any (?) the Addrinfo wrapper struct was dropped, because of IP4/IP6 issues and it's not particularly useful, unless we want to open a public API (let's see that later);
  • specs ! this is a private API, socket specs already test that it works;
  • generate bindings for the AI_* and EAI_* LibC constants for each supported target.

I was wondering about having different resolvers under DNS that let applications use the strategy they want, since none is perfect. For example a blocking resolver that merely calls getaddrinfo; a threaded resolver that uses threads to call getaddrinfo (ie. this PR, should be the default); a resolver that would link against c-ares for applications that don't care about libc extensions and want a real async DNS resolver (eg: servers).

Note that this solution may be dropped and no longer be the default when the event-loop will become multithreaded. Limiting the number of concurrent calls to getaddrinfo in order to not block all threads (like go does) may be preferable.

refs #2745 #2660

Sorry, something went wrong.


def initialize
@que = Deque(T).new(16)
@mutex = Thread::Mutex.new
Copy link
Contributor Author

@ysbaddaden ysbaddaden Jun 14, 2016

Choose a reason for hiding this comment

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

Using a mutex is easy, but maybe not optimal in terms of speed. Using http://llvm.org/releases/3.3/docs/LangRef.html#cmpxchg-instruction and http://llvm.org/releases/3.3/docs/LangRef.html#atomicrmw-instruction for acquiring/releasing a lock may lead to better performance.

Copy link
Contributor Author

@ysbaddaden ysbaddaden Jun 14, 2016

Choose a reason for hiding this comment

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

For example try to lock with cmpxchg(counter, 1, 0) and yielding the fiber until it succeeds, then release the lock with modifyrmw(counter, sub, counter, 1). We'd need to differentiate between push/pop/clear from the event-loop thread (there is a fiber) and calling from a thread (no fiber) and act accordingly; maybe using a semaphore from the thread.

This may be too complex, thought, and we should just not care until we tackle parallelism.

@asterite
Copy link
Member

Thanks! I think this will be part of the solution that eventually will remain in the standard library (that is, firing a thread and invoking getaddrinfo there). I'd still like to wait @waj to check this, as he knows pretty well what works and not when firing a new thread. I remember using fibers there didn't work (eventually crashed or the GC entered an allocation loop), not sure if not using fibers but allocating memory or communicating with other threads is OK though (what this PR does). Hopefully we can review and finish this for 0.19.0 :-)

loop do
@mutex.synchronize do
if @que.empty?
raise Error.new("queue is empty") unless blocking
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When called from the event-loop, this should always non blocking, otherwise the next call will block the event loop until a result a available (bad).

It could also merely return nil, allowing the calling Fiber to yield itself, instead of spending resources raising an exception, catching it and then yielding. Thought there won't be any mean to differentiate between a "would block" and a "popping nil" scenarios.

@ysbaddaden
Copy link
Contributor Author

Memory usage is stable over time (on linux glibc) when resolving localhost in a tight loop. I have a feeling that LLVM frees local variables immediately, or BOEHM GC does its job correctly. Also, the threads never make any call that will result into accessing the event loop (libevent's base), they merely use C syscalls: wait on a Mutex/ConditionVariable, call getaddrinfo and nothing more.

@ysbaddaden ysbaddaden force-pushed the dns-threaded-resolver branch from be332b4 to 5064997 Compare June 20, 2016 12:36
@ysbaddaden
Copy link
Contributor Author

I made some modifications (with new commits):

  • I dropped the Addrinfo wrapper because I ran into issues with the sockaddr C structs, and it's not required for internal stuffs (except to introduce a lot of useless copying). Maybe it could become a nice public API, but let's see that later and keep it private for now.
  • I dropped the DNS namespace, and moved the threaded resolver to Socket::Addrinfo. I believe it's a better place for them.
  • I made some additional cleanup in the socket classes: harmonized coding styles and variable names, added some missing methods to UNIXServer, along with some basic documentation and a few fixes.

Still a work in progress. I must generate POSIX bindings for the remaining supported targets, and squash things a bit.

@ysbaddaden ysbaddaden force-pushed the dns-threaded-resolver branch from 5064997 to 41be0e6 Compare August 1, 2016 20:13
@ysbaddaden ysbaddaden mentioned this pull request Aug 1, 2016
@sdogruyol
Copy link
Member

Bump on this 👍

@Sija
Copy link
Contributor

Sija commented Mar 24, 2017

Bump again.

@bararchy
Copy link
Contributor

@ysbaddaden @RX14 @straight-shoota
Should we maybe revisit this idea?

@ysbaddaden
Copy link
Contributor Author

No, the ability to configure the resolver in #4236 is better.

@bararchy
Copy link
Contributor

@ysbaddaden makes sense.
Should we maybe close this one then ?

@ysbaddaden ysbaddaden closed this Mar 29, 2018
@ysbaddaden ysbaddaden deleted the dns-threaded-resolver branch March 29, 2018 08:16
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.

None yet

5 participants