RFR JDK-8153353: HPACK implementation

[Roger Riggs]

Hi Pavel,

On 4/12/2016 10:36 AM, Pavel Rappo wrote:
> Hi Roger, thanks for looking into this!
>
> Changes are done in-place.
It saves hunting around if you provide the link.

http://cr.openjdk.java.net/~prappo/8153353/webrev.00/

>
> Do you think issues marked as [*] could be addressed incrementally after the
> initial push?
>
>> On 11 Apr 2016, at 16:18, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>>
>> Hi Pavel,
>>
>> Though this is  an implementation only package, it could use a few more comments
>> to support maintainers.  In the package.html, a couple of hints about what classes are
>> the external interface would be useful and an example of use in Encode and/or Decode
>> would help a lot.
> [*]
>
>> The tests use randomness and should have @key randomness in the tests.
>>
>> To allow reproducibility, the tests should print the seeds used and be able to be restarted
>> with a seed.  The testlibrary has a convenient factory for Random that logs the seed.
>>
>> See jdk.testlibrary.RandomFactory
>>
>> * @library /lib/testlibrary/
>> * @build jdk.testlibrary.RandomFactory
>>
>> and use it as:
>>     private static final Random random = RandomFactory.getRandom();
> Since these tests are "white box", I find it hard to inject
> jdk.testlibrary.RandomFactory into them, given they are compiled-in
> java.httpclient module.
>
> Would it be an acceptable work around to manually log-and-set seeds for any PRNG
> used in tests?
>
> 113     private final Random rnd;
> 114     {
> 115         long seed = System.currentTimeMillis();
> 116         System.out.println(seed);
> 117         rnd = new Random(seed);
> 118     }
Sure, there is nothing clever about the test library version.

If you can't import the test library version, then copy/paste the code 
so you get all of its
functions including being able to set the seed via a system property on 
the command line.

Since you have a TestHelper class you could put it there and not 
duplicate the code in several tests.
>> IntegerReader:
>>   - It would be useful to have a reference to the spec for variable length integers: i.e.  rfc 7541 5.1 Integer Representation
> [*] I have it in my TODO list.
it could be as simple as referring to "RFC 7541 section 5.1 Integer 
Representation."
>
>> - Line 129, 53: inconsistent IAE's in some the valid condition is included in the exception message,
>>    but in others the *in*valid condition is included but without any indication of which is which.
>>
>> - line 109/110:  style put 'while' on same line as '}' from do so it does not look like a separate statement.
>>
>> IntegerWriter:
>> - line 40:  perhaps 'value' or 'v' would be more readable than 'i' which is often thought of as a local value.
> Done.
ok
>
> I've also converted JUnit tests into TestNG ones, as it's de facto standard in
> core-libs tests.
good
> Fixed remainder operator '%' mentioned by Simone, which I guess
> now reads better (and explained the intention as an assert):
>
> 474     public int lengthOf(CharSequence value, int start, int end) {
> 475         int len = 0;
> 476         for (int i = start; i < end; i++) {
> 477             char c = value.charAt(i);
> 478             len += INSTANCE.codeOf(c).length;
> 479         }
> 480         // Integer division with ceiling, assumption:
> 481         assert (len / 8 + (len % 8 != 0 ? 1 : 0)) == (len + 7) / 8 : len;
You don't believe the expression is equivalent?
> 482         return (len + 7) / 8;
> 483     }
:)

Thanks, Roger

>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160412/b2a41f03/attachment.html>