quicvarint: add Reader and Writer interfaces #3233

ydnar · 2021-07-19T19:03:26Z

This PR adds two interfaces to the quicvarint package: Reader and Writer, that are io.Reader + io.ByteReader, and io.Writer + io.ByteWriter, respectively, along with constructors that pass through the argument unchanged if it implements the necessary interfaces.

The interfaces to quicvarint.Read and Write are changed to accept a Reader and Writer, respectively. This change didn’t break tests in any package in this repo, nor require any changes in any package that depends on quicvarint.

Why? This is a small bit of support work for #3226, to ease construction of a package that processes H3 streams and datagrams outside of the http3 package.

This PR also removes the existing http3.byteReaderImpl type, replacing it with quicvarint.Reader.

Read doesn’t currently use io.Reader interface. This was done for symmetry.

marten-seemann · 2021-07-20T18:58:19Z

Thank you for this PR.
Do you have any benchmarks for this change? The thing that worries me the most are the calls to Read and Write, as these functions are called a lot. If I understand correctly, NewReader and NewWriter shouldn't allocate when passed a bytes.Reader or a bytes.Writer, respectively (which is how we commonly use it for parsing the header and the frames).
Given that the actual work done in Read and Write is quite trivial though, my fear is that the type assertions might show up in a benchmark.

codecov · 2021-07-20T19:02:05Z

Codecov Report

Merging #3233 (bda11a9) into master (4e166bb) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3233      +/-   ##
==========================================
- Coverage   85.46%   85.44%   -0.01%     
==========================================
  Files         133      134       +1     
  Lines        9799     9811      +12     
==========================================
+ Hits         8374     8383       +9     
- Misses       1052     1053       +1     
- Partials      373      375       +2

Impacted Files	Coverage Δ
http3/client.go	`89.35% <100.00%> (ø)`
http3/frames.go	`78.38% <100.00%> (-1.62%)`	⬇️
http3/server.go	`69.40% <100.00%> (ø)`
quicvarint/io.go	`100.00% <100.00%> (ø)`
quicvarint/varint.go	`78.38% <100.00%> (ø)`
conn_oob.go	`60.53% <0.00%> (-0.70%)`	⬇️
internal/ackhandler/sent_packet_handler.go	`78.01% <0.00%> (-0.05%)`	⬇️
internal/flowcontrol/stream_flow_controller.go	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e166bb...bda11a9. Read the comment docs.

ydnar · 2021-07-20T21:04:33Z

Do you have any benchmarks for this change? The thing that worries me the most are the calls to Read and Write, as these functions are called a lot. If I understand correctly, NewReader and NewWriter shouldn't allocate when passed a bytes.Reader or a bytes.Writer, respectively (which is how we commonly use it for parsing the header and the frames).
Given that the actual work done in Read and Write is quite trivial though, my fear is that the type assertions might show up in a benchmark.

When I ran the existing benchmarks suite, it seemed on par. I can add benchmarks for quicvarint.Read and Write if that’d be helpful.

If the type assertion does negatively impact the benchmarks, what do you think about changing the method signatures to accept a quicvarint.Reader or Writer instead of an io.Reader?

marten-seemann · 2021-07-20T22:40:39Z

When I ran the existing benchmarks suite, it seemed on par. I can add benchmarks for quicvarint.Read and Write if that’d be helpful.

Our benchmark package is a few years old and frankly, it's not very useful any more. I was thinking of a microbenchmark just for the Read and Write function here.

If the type assertion does negatively impact the benchmarks, what do you think about changing the method signatures to accept a quicvarint.Reader or Writer instead of an io.Reader?

I wouldn't be opposed to that. I assume it doesn't break any (quic-go) code, does it?

ydnar · 2021-07-21T00:47:44Z

When I ran the existing benchmarks suite, it seemed on par. I can add benchmarks for quicvarint.Read and Write if that’d be helpful.

Our benchmark package is a few years old and frankly, it's not very useful any more. I was thinking of a microbenchmark just for the Read and Write function here.

Sounds good. Would you prefer a gomega/ginkgo or vanilla Go benchmark?

If the type assertion does negatively impact the benchmarks, what do you think about changing the method signatures to accept a quicvarint.Reader or Writer instead of an io.Reader?

I wouldn't be opposed to that. I assume it doesn't break any (quic-go) code, does it?

It didn’t, since every existing caller passes a *bytes.Buffer to Write, which implements quicvarint.Writer and Reader.

- Read(io.Reader) with type assertion to quicvarint.Reader - Read(io.ByteReader) with no type assertion (current master branch) - Read(quicvarint.Reader) with no type assertion Results: the current Read func in this branch is approximately 86% slower (1.86x) runtime than Read on current master branch. The other implementation which accepts a quicvarint.Reader with no type assertion is on par with master. Given this function is in the hot path, I recommend we eliminate the type assertion in favor of the caller ensuring argument to Read() satisfies the quicvarint.Reader or io.ByteReader interface.

ydnar · 2021-07-21T03:59:12Z

@marten-seemann looks like you’re right. The type assertion exacts a material penalty in the Read call, almost doubling its execution time:

❯ go test -bench . ./quicvarint
Running Suite: QUIC Varint Suite
================================
Random Seed: 1626839707
Will run 40 of 40 specs

••••••••••••••••••••••••••••••
------------------------------
• [MEASUREMENT]
Benchmarks
  with type assertion to quicvarint.Reader
    reading from a bytes.Reader

    Ran 100 samples:
    read:
      Fastest Time: 0.000s
      Slowest Time: 0.000s
      Average Time: 0.000s ± 0.000s
    read one (ns):
      Smallest: 17.913
       Largest: 64.634
       Average: 21.401 ± 5.843
    read all (ns):
      Smallest: 5732.000
       Largest: 20683.000
       Average: 6848.420 ± 1869.883
------------------------------
• [MEASUREMENT]
Benchmarks
  old Read(io.ByteReader) without type assertion
    reading from a bytes.Reader

    Ran 100 samples:
    read:
      Fastest Time: 0.000s
      Slowest Time: 0.000s
      Average Time: 0.000s ± 0.000s
    read one (ns):
      Smallest: 10.466
       Largest: 20.159
       Average: 11.693 ± 1.975
    read all (ns):
      Smallest: 3349.000
       Largest: 6451.000
       Average: 3741.860 ± 631.840
------------------------------
• [MEASUREMENT]
Benchmarks
  new Read(Reader) without type assertion
    reading from a bytes.Reader

    Ran 100 samples:
    read:
      Fastest Time: 0.000s
      Slowest Time: 0.000s
      Average Time: 0.000s ± 0.000s
    read one (ns):
      Smallest: 10.637
       Largest: 63.631
       Average: 11.613 ± 5.237
    read all (ns):
      Smallest: 3404.000
       Largest: 20362.000
       Average: 3716.000 ± 1675.817
------------------------------
••••••
Ran 40 of 40 Specs in 0.010 seconds
SUCCESS! -- 39 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS
ok  	github.com/lucas-clemente/quic-go/quicvarint	0.145s

Type algebra says that if r (type io.Reader) implements io.Reader and io.ByteReader, it implements Reader. Therefore the reader and writer structs are not needed, and can be eliminated.

This improves performance per call of Read by ~46% TODO: change parseNextFrame to accept quicvarint.Reader

marten-seemann

Thank you for the benchmarks!

I have to admit I'm a bit confused now about the interfaces changes, see my comments on the code.

quicvarint/varint.go

PR feedback

ydnar · 2021-07-22T13:41:50Z

Unrelated: do you plan to remove the CircleCI and Travis checks?

ydnar · 2021-07-26T18:33:42Z

@marten-seemann I put up a PR (#3238) that builds on this PR.

Do you want to review or merge this one?

marten-seemann

One small comment, then this should be good to merge.
Can you please remove the benchmark file? Ginkgo has deprecated the benchmark functionality anyway.

quicvarint/varint.go

ydnar · 2021-08-05T17:00:37Z

Can you please remove the benchmark file? Ginkgo has deprecated the benchmark functionality anyway.

Done.

This was my first time using Ginkgo. I normally write vanilla Go tests. What drove the decision to use it?

ydnar added 7 commits July 19, 2021 11:29

quicvarint: add Reader and Writer interfaces

c91d367

quicvarint: use Writer interface instead of *bytes.Buffer

96f3658

quicvarint: use Reader instead of io.ByteReader

1de0b36

Read doesn’t currently use io.Reader interface. This was done for symmetry.

http3: use quicvarint.Reader

0ff8070

quicvarint: update doc comments

c49f70f

quicvarint: use io.Reader and io.Writer at top-level

3b51c5f

http3: remove quicvarint.Reader; not necessary anymore

412b5f1

ydnar added 2 commits July 20, 2021 20:49

quicvarint: reorder RecordValue calls (one->all)

3b27076

ydnar added 3 commits July 20, 2021 21:17

quicvarint: remove useless wrapper structs

d784d4c

Type algebra says that if r (type io.Reader) implements io.Reader and io.ByteReader, it implements Reader. Therefore the reader and writer structs are not needed, and can be eliminated.

quicvarint: Write(Writer) to match WriteWithLen

75754b8

quicvarint, http3: remove type assertion from quicvarint.{Read,Write}

1679e2a

This improves performance per call of Read by ~46% TODO: change parseNextFrame to accept quicvarint.Reader

marten-seemann reviewed Jul 21, 2021

View reviewed changes

quicvarint/varint.go Outdated Show resolved Hide resolved

quicvarint/varint.go Show resolved Hide resolved

ydnar and others added 3 commits July 21, 2021 08:05

quicvarint: benchmark a Read impl using io.Reader

f0666db

quicvarint: make benchmarks consistent

dd20364

quicvarint: accept a smaller interface (io.ByteReader)

8b09526

PR feedback

ydnar mentioned this pull request Jul 26, 2021

http3: add exported Settings type #3238

Closed

ydnar mentioned this pull request Jul 26, 2021

[draft] support WebTransport #3226

Closed

6 tasks

marten-seemann approved these changes Aug 5, 2021

View reviewed changes

quicvarint/varint.go Outdated Show resolved Hide resolved

ydnar added 3 commits August 5, 2021 09:26

quicvarint: remove unnecessary NewWriter

47f51f8

quicvarint: remove benchmark

a1bebff

quicvarint: use byte array on stack instead of make([byte)

bda11a9

marten-seemann approved these changes Aug 5, 2021

View reviewed changes

marten-seemann merged commit 346bd63 into quic-go:master Aug 5, 2021

aschmahmann mentioned this pull request Aug 23, 2021

Release v0.10 ipfs/kubo#8176

Closed

62 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quicvarint: add Reader and Writer interfaces #3233

quicvarint: add Reader and Writer interfaces #3233

ydnar commented Jul 19, 2021 •

edited

Loading

marten-seemann commented Jul 20, 2021

codecov bot commented Jul 20, 2021 •

edited

Loading

ydnar commented Jul 20, 2021

marten-seemann commented Jul 20, 2021

ydnar commented Jul 21, 2021

ydnar commented Jul 21, 2021

marten-seemann left a comment

ydnar commented Jul 22, 2021

ydnar commented Jul 26, 2021

marten-seemann left a comment

ydnar commented Aug 5, 2021

quicvarint: add Reader and Writer interfaces #3233

quicvarint: add Reader and Writer interfaces #3233

Conversation

ydnar commented Jul 19, 2021 • edited Loading

marten-seemann commented Jul 20, 2021

codecov bot commented Jul 20, 2021 • edited Loading

Codecov Report

ydnar commented Jul 20, 2021

marten-seemann commented Jul 20, 2021

ydnar commented Jul 21, 2021

ydnar commented Jul 21, 2021

marten-seemann left a comment

Choose a reason for hiding this comment

ydnar commented Jul 22, 2021

ydnar commented Jul 26, 2021

marten-seemann left a comment

Choose a reason for hiding this comment

ydnar commented Aug 5, 2021

ydnar commented Jul 19, 2021 •

edited

Loading

codecov bot commented Jul 20, 2021 •

edited

Loading