The AsyncIO Server should use Python's 3.7 BufferedProtocol when possible ?

[tzickel]

Not sure how much performance it can add, but might be worth exploring:
https://bugs.python.org/issue32251

It can be done in a backward compatible way with Python 3.6 like this:
https://github.com/huge-success/sanic/blob/bedf68a9b2025618a94cb8044f495a0abd87a134/sanic/server.py#L47

Instead of inheriting asyncio.Protocol, it can be done:

try:
    from asyncio import BufferedProtocol as BaseProtocol
# Python 3.6 support
except ImportError:
    from asyncio import Protocol as BaseProtocol

And inside the implementation, it can be written for both Python 3.6 and 3.7+ by defining the 3 methods (where the first 2 use a preallocated buffer):

class HttpProtocol(BaseProtocol):
    def get_buffer(self, sizehint):
    def buffer_updated(self, nbytes):
    # Python 3.6 support
    def data_received(self, data):

[Tronic]

Did you check if uvloop also supports this and whether there is any benefit of using BufferedProtocol? Sanic uses uvloop instead of asyncio whenever possible, as that runs much faster but provides mostly identical API.

Do notice that future development of Sanic is planned based on the streaming branch, which completely rewrites the code that you are referring to.

I'd love to see a PR with benchmarks, against the streaming branch.

[ahopkins]

@tzickel any interest in pursuing this further?

[tzickel]

Sure on which branch to check ?

[ahopkins]

אפשר בשלך.

You can make a fork of the repo, then branch off master in your fork. When you are done make a PR back into the main master. Let me know if you have any questions. We can worry about streaming at a later date.

[tzickel]

Hmm... Doesn't seem to matter that much from my testing (unless I did something wrong) :/

diff --git a/sanic/server.py b/sanic/server.py
index 6adf5dd..ffd60fe 100644
--- a/sanic/server.py
+++ b/sanic/server.py
@@ -31,6 +31,10 @@ from sanic.log import access_logger, logger
 from sanic.request import EXPECT_HEADER, Request, StreamBuffer
 from sanic.response import HTTPResponse
 
+try:
+    from asyncio import BufferedProtocol as BaseProtocol
+except ImportError:
+    from asyncio import Protocol as BaseProtocol
 
 try:
     import uvloop  # type: ignore
@@ -82,7 +86,7 @@ class ConnInfo:
             self.client_port = addr[1]
 
 
-class HttpProtocol(asyncio.Protocol):
+class HttpProtocol(BaseProtocol):
     """
     This class provides a basic HTTP implementation of the sanic framework.
     """
@@ -129,6 +133,7 @@ class HttpProtocol(asyncio.Protocol):
         "state",
         "_unix",
         "_body_chunks",
+        "_buffer",
     )
 
     def __init__(
@@ -142,6 +147,7 @@ class HttpProtocol(asyncio.Protocol):
         unix=None,
         **kwargs,
     ):
+        self._buffer = memoryview(bytearray(64 * 1024))
         asyncio.set_event_loop(loop)
         self.loop = loop
         deprecated_loop = self.loop if sys.version_info < (3, 7) else None
@@ -289,6 +295,12 @@ class HttpProtocol(asyncio.Protocol):
     # Parsing
     # -------------------------------------------- #
 
+    def get_buffer(self, sizehint=-1):
+        return self._buffer
+
+    def buffer_updated(self, nbytes):
+        self.data_received(self._buffer[:nbytes])
+
     def data_received(self, data):
         # Check for the request itself getting too large and exceeding
         # memory limits

[ahopkins]

Well, I guess it depends on what and how you are testing. If there is one thing I have learned since being a part of Sanic, it is that performance testing on a web framework is not very accurate and highly conditional.

[ahopkins]

I did a fairly crude experiment with this. I ran master against my benchmarker.

First, the test ran on a very small response payload. A "hello world" type of response. Using Protocol I hit around 127K req/s. Using BufferedProtocol, it was about 130K req/s. This is definitely within my margin of error, and itself doesn't mean too much without a more in depth analysis.

But, when I tested it against a payload of about 250KB, there was a significant difference. It jumped from 119K req/s to 127.5K req/s. That leads me to believe that for larger payloads there is a potential performance increase (or at least not a penalty) that this should warrant another look and inclusion into 21.3.

[ahopkins]

Unless someone can prove to me this is faster, I am inclined to close this one.