Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    Nathaniel J. Smith
    @njsmith
    I know that you don't have a version without rstrip that works totally correctly, but it should be close enough to run the benchmark
    cdeler
    @cdeler
    Yes, sure, I can benchmark master vs .rstrip() vs no-rstrip() versions
    Nathaniel J. Smith
    @njsmith
    And for I really mean a quick and dirty comparison between your no-rstrip versus the exact same code but with just the rstrip added
    No need to get elaborate; the idea is just to find out quickly whether rstrip matters at all
    cdeler
    @cdeler

    Without rstrip():

    6606.1 requests/sec
    6579.0 requests/sec
    6566.2 requests/sec
    6613.8 requests/sec
    6599.3 requests/sec
    6609.9 requests/sec
    6499.8 requests/sec

    With rstrip()

    6236.0 requests/sec
    6307.3 requests/sec
    6508.0 requests/sec
    6534.3 requests/sec
    6448.1 requests/sec
    6493.6 requests/sec
    6482.0 requests/sec
    6609 (without) > 6534 (with)
    cdeler
    @cdeler

    So you're totally right; the issue is in this function: https://github.com/python-hyper/h11/blob/master/h11/_readers.py#L35

    Thank you for advice about the desired spot, I feel so stupid!

    I rewrote _obsolete_line_fold a bit:

    def _obsolete_line_fold(lines):
        it = iter(lines)
        last = None
        for line in it:
            match = obs_fold_re.match(line)
            if match:
                if last is None:
                    raise LocalProtocolError("continuation line at start of headers")
                if not isinstance(last, bytearray):
                    last = bytearray(last)
    
                while last.endswith(b"\r"):  # change is here
                    del last[-1]             # and there
    
                last += b" "
                last += line[match.end() :]
            else:
                if last is not None:
                    yield last
                last = line
        if last is not None:
            while last.endswith(b"\r"):  # change is here
                del last[-1]  # and there
    
            yield last

    I assume that if assert isinstance(last, bytearray), then we can just remove the end of the bytearray (please correct me if I'm wrong, but it shouldn't cause an extra alloc).

    Nathaniel J. Smith
    @njsmith
    yeah, that should be correct
    it's the first change that really makes the difference, since that's what's needed to avoid ending up with \r in the middle of a header value
    the second change only removes \r from the end of header values, and that's also handled by the regexp changes :-)
    alternatively I guess we could drop those regexp changes and just do if line[-1] == b"\r": del line[-1] at the top of the for loop there
    Nathaniel J. Smith
    @njsmith
    or similar in maybe_extract_lines
    (basically implementing a cheaper form of rstrip by avoiding the copy)
    tbh pretty much any of these could work, we should probably just pick one and move on :-)
    (and btw, don't feel stupid -- the stupid thing is how complicated it is to parse http/1)
    cdeler
    @cdeler
    I'd to add _cheap_rstrip to ReceiveBuffer not to spread delimiter logic across the project.
    def _cheap_rstrip(line):
        if line.endswith(b"\r"):
            del line[-1]
    
        return line
    
    
    class ReceiveBuffer(object):
        ...
        def maybe_extract_lines(self):
            ...
            lines = list(map(_cheap_rstrip, out.split(b"\n")))
    Nathaniel J. Smith
    @njsmith
    might be even clearer to write it like:
    lines = out.split(b"\n")
    for line in lines:
        if line[-1] == b"\r":
            del line[-1]
    cdeler
    @cdeler

    bench.py results are the same.
    rstrip() version:

    6444.4 requests/sec
    6271.3 requests/sec
    6463.9 requests/sec
    6517.6 requests/sec
    6467.8 requests/sec
    6484.8 requests/sec
    6461.5 requests/sec

    del line[-1] version

    6477.9 requests/sec
    6495.6 requests/sec
    6508.1 requests/sec
    6474.8 requests/sec
    6473.1 requests/sec
    6513.7 requests/sec
    6504.7 requests/sec
    Nathaniel J. Smith
    @njsmith
    ok, let's go with the del line[-1] version because it's less likely to have surprising performance effects in case people have like, extra large headers or something
    and then we can drop the regexp changes
    @cdeler and thanks for your patience with all this :-)
    cdeler
    @cdeler
    I'll do that in 2 hours (as I reach my PC)
    Nathaniel J. Smith
    @njsmith
    :+1:
    cdeler
    @cdeler

    @njsmith
    I've done with it.

    Thank you a lot for advising! It's really interesting and challenging to write effective code. Live and learn

    cdeler
    @cdeler

    Hello @njsmith

    I'm sorry that I again disturb you, but please could you answer?
    What can we do to merge and release python-hyper/h11#115 ?

    I don't want to be so irritating , just wonder if I can help somehow

    Phil Jones
    @pgjones

    Consistent Hyper linting

    I would like to accept python-hyper/h11#117 and more generally setup flake8 linting with the same settings in all the projects (probably https://gitlab.com/pgjones/quart/-/blob/master/tox.ini#L32 which I use in most places).

    I'll take apathy as approval :), but would value any suggestions.

    Nathaniel J. Smith
    @njsmith
    I find flake8 more annoying than useful personally, but I think you should do what makes you happy :-)
    cdeler
    @cdeler

    Hello!

    Happy New Year everyone!
    Do you know whenever it's possible to release h11? Having released h11, we can create a NewYear gift to the axis community (https://github.com/home-assistant/core/issues/42415#issuecomment-752667227)

    Phil Jones
    @pgjones
    I think we should complete the Py3 transition first, (python-hyper/h11#124 or python-hyper/h11#118 ), maybe python-hyper/h11#125, and add the type hints.
    We don't have to wait for these though, and I see it would be nice that we didn't. I have time on Friday to release.
    Robert Svensson
    @Kane610
    Wow! That would be great @pgjones!
    Thomas Kriechbaumer
    @Kriechi
    @njsmith any thoughts on black over flake8? I've seen it work quite nicely in a few projects - but it is a significant mental investment to fully hand off control over how the code is formatted
    Phil Jones
    @pgjones
    We've already adopted black in all the projects, I think.
    Nathaniel J. Smith
    @njsmith
    black is fantastic
    Thomas Kriechbaumer
    @Kriechi
    if we already adopted it, then why introduce flake8?
    Phil Jones
    @pgjones
    It tends to catch stupid mistakes I make, like imports I've forgotten to delete, print statements I've left in the code, etc...
    Thomas Kriechbaumer
    @Kriechi
    no major objection - but I thought one tools should be enough... black or flake8...
    Nathaniel J. Smith
    @njsmith
    flake8 does more than formatting
    Phil Jones
    @pgjones
    python-hyper/h11#126 for release 0.12.0
    Nathaniel J. Smith
    @njsmith
    :tada:
    Phil Jones
    @pgjones
    @Kane610 @cdeler a new (year) release, 0.12.0 is on PyPI. Looks like httpcore (httpx) will pick it up, so hopefully your issue is solved.
    cdeler
    @cdeler
    Thank you! Happy New Year! :cocktail:
    Thomas Kriechbaumer
    @Kriechi
    should we put https://github.com/python-hyper/hyper into Archive mode? its heavily outdated any nobody seems motivated enough to maintain it
    Nathaniel J. Smith
    @njsmith
    I was wondering that too
    Phil Jones
    @pgjones
    Does seem sensible, maybe update the readme to point at httpx?
    Robert Svensson
    @Kane610
    @pgjones awesome 🎉thanks ! And an extra thank you to @cdeler. And a happy new year to you all
    Thomas Kriechbaumer
    @Kriechi
    @pgjones yes. done.
    and I set the repo into read-only archive mode.
    Phil Jones
    @pgjones
    :+1:
    Thanks