Re: [tsvwg] Benjamin Kaduk's No Objection on draft-ietf-tsvwg-rlc-fec-scheme-14: (with COMMENT)

[Roman Danyliw <[e-mail]>]

Hi!

I’m keeping the subject line but we’re really talking about draft-ietf-tsvwg-tinymt32 here.

Your changes in -04 addresses all of my DISCUSSes and almost all of my COMMENTs.  Thanks for the update.  Adam is holding a DISCUSS that’s nearly identical to my remaining COMMENT (about having a normative reference for C99) so I’ll let him hold the issue.

More details inline …

From: iesg [mailto:[e-mail]] On Behalf Of Vincent Roca
Sent: Wednesday, June 12, 2019 12:16 PM
To: The IESG <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]
Subject: Re: [tsvwg] Benjamin Kaduk's No Objection on draft-ietf-tsvwg-rlc-fec-scheme-14: (with COMMENT)

IESG Members, all,

Thanks a lot for this second IESG review round (last time it was part of the RLC I-D).
I have copied below all Discuss/Comments made for this -03 version of the document
(previous comments for -02 have already been considered) and added our answers.

Many thanks, the document quality has been further improved.
Cheers,

   Vincent, on behalf of the authors


==========

> Roman Danyliw Discuss
>
> Discuss (2019-05-28)
>
> (1) Section 1.  Per “The TinyMT32 PRNG initialization depends, among other things, on a parameter set -- namely (mat1, mat2, tmat) -- that needs to be well chosen (pre-calculated values are available in the official web site).”, why is there a reference to an external website when Section 3.1 explicitly lists the constant and mandates their usage?  I’m elevating this to a discuss as there should be no ambiguity on the location of these constants.

[VR] You're right, it brings confusion. We have removed this
part of the sentence.

NEW:
   The TinyMT32 PRNG initialization depends, among other things, on a
   parameter set, namely (mat1, mat2, tmat).  In order to facilitate the
   use of this PRNG and make the sequence of pseudo-random numbers
   depend only on the seed value, this specification requires the use of
   a specific parameter set (see Section 3.1).  This is a major
   difference with respect to the implementation version 1.1
   (2015/04/24) that leaves this parameter set unspecified.


[Roman] Thanks.  This addresses my feedback.

> (2) Section 3.1.  A comment in tinymt32_next_state() notes code changes made to the “original code … [so it is] not depend on the representation of negative integers by 2's complements.”  The following code in tinymt32_temper() appears to suffer from this same, original portability issue:
> t0 ^= -((int32_t)(t1 & 1)) & s->tmat;

[VR] Shame on me, I've omitted this line.
You're perfectly right and here is the fix. Thanks!

NEW:
       /*
        * The if (t1 & 1) {...} block below replaces:
        *     t0 ^= -((int32_t)(t1 & 1)) & s->tmat;
        * The adopted code is equivalent to the original code
        * but does not depend on the representation of negative
        * integers by 2's complements. It is therefore more
        * portable, but includes an if-branch which may slow
        * down the generation speed.
        */
       if (t1 & 1) {
           t0 ^= s->tmat;
       }

[Roman] Thanks.  This addresses my feedback.

==========

> Comment (2019-05-28)
>
> (1) Section 1.  “BigCrush in TestU01 and AdaptiveCrush” needs a reference.

[VR] I have added the two URLs for the academic publicatons behind these
two PRNG statistical tests, keeping also the two URLs to the related
software web sites.

[Roman]  Thanks.  This addresses my feedback.

> (2) Abstract and Section 1.  I found characterizing the quality of the randomness as “pretty good” imprecise (relative to what?).  Can this statement be qualified or qualified in some way?

[VR] Yes, I've clarified the abstract by refering to Park-Miller PRNG.
I think the new Section I paragraph contains enough information to
further support this claim.

NEW (abstract):
   The main
   advantage of TinyMT32 over MT is the use of a small internal state,
   compatible with most target platforms including embedded devices,
   while keeping a reasonably good randomness that represents a
   sigificant improvement compared to the Park-Miller Linear
   Congruential PRNG.

[Roman]  Thanks.  This addresses my feedback.

> (3) Section 1 (“This is the first difference …”) and Section 3.1 (“The TinyMT32 PRNG reference implementation is reproduced in Figure 1, with the following differences …”) both enumerate a list of differences between the github repo and the code in this text.  Why are both required? Section 3.1 seems more precise.

[VR] You're right, we now only refer to the specialization of the
(mat1, mat2, tmat) tuple in Section 1 (because of its importance in
the way we use the PRNG), leaving the details for Section 3.1.

[Roman]  Thanks.  This addresses my feedback.

> (4) Section 1.  I am confused by the terminology of “official web site” and “official github site”.  What makes them official?

[VR] Right, and we changed the wording:

NEW:

   This document specifies the TinyMT32 PRNG, as a specialization of the
   reference implementation version 1.1 (2015/04/24) by Mutsuo Saito and
   Makoto Matsumoto, from Hiroshima University, that can be found at
   [TinyMT-web] (TinyMT web site) and [TinyMT-dev] (Github site).  This
   specialisation aims at having a simple-to-use and deterministic PRNG,
   as explained below.

[Roman]  Thanks.  This addresses my feedback.

> (5) Section 1.  With due respect to the first two authors, it seems odd to mention them by name three times in this section.  Also, the fact that this document is derived from a github repo is mentioned twice in this section and once in the code comments of Section 3.1.

[VR] Yes, you're right. Their names now appear only once in text, once in source code, and of course in the authors list and in references. This is fair.

[Roman]  Thanks.  This addresses my feedback.

> (6) Section 3.1.  There are normative C code blocks in this text.  The obvious fact that this is C code needs to be specified – consider C99 (ISO/IEC 9899:1999), C11 (ISO/IEC 9899:2011), C18 (ISO/IEC 9899:2018)

[VR] Yes, you're right. I've checked with the -c99, -c11, -c17
(I'm using CLANG and -c18 does not exist yet) C dialect flags
and it compiles/runs without any problem.

NEW:
   The TinyMT32 PRNG reference implementation is reproduced in Figure 1.
   This is a C language implementation, compatible with the C99 (ISO/IEC
   9899:1999), C11 (ISO/IEC 9899:2011) and C18 (ISO/IEC 9899:2018)
   versions of the C language.  ...

[Roman]  This language correct, but it needs to explicitly cite one of these a normative reference.  FWIW, I think you can just say C99.

> (7) Section 3.1.  I agree with ekr’s original comment that hard coded a constant in tinymt32_init() makes the code less readable -- “s->status[i & 3] ^= i + UINT32_C(1812433253)”

[VR] In general I would agree but in this particular case I have
no idea what this constant means. Hard to assign it a meaningful
constant name. Is that really a problem?

[Roman]  No problem.

> (8) Editorial Nits
>
> Abstract.  No references are permitted in the abstract.

[VR] removed (the reference still appears in Section I so this is not an issue).

> Section 1.  Typo.  s/variatnt/variant/
> Section 1.  Typo.  s/randomnes/randomness/
> Section 3.1.  Typo.  s/licence/license/

[VR] done.

[Roman]  Thanks!

Regards,
Roman