[m-rev.] for review: reading multibyte integers from binary file streams
Julien Fischer
jfischer at opturion.com
Sun Jan 20 15:36:28 AEDT 2019
I'm going to go ahead and commit this one now; I'll deal with any further
review comments separately.
Julien.
On Tue, 15 Jan 2019, Julien Fischer wrote:
>
> Hi Zoltan,
>
> My responses to your review are below; there are relative and full diffs
> attached.
>
> On Wed, 9 Jan 2019, Zoltan Somogyi wrote:
>
>> On Tue, 8 Jan 2019 04:02:56 +0000 (UTC), Julien Fischer
>> <jfischer at opturion.com> wrote:
>>>> I will have a look if you send me the diff as an attachment, though
>>>> someone else should check the Java and C# versions.
>>>
>>> Attached.
>>
>> And the review is attached to this email.
>
>> > diff --git a/library/io.m b/library/io.m
>> > index d407c09..4b852c7 100644
>> > --- a/library/io.m
>> > +++ b/library/io.m
>> > @@ -108,6 +108,16 @@
>> > ; eof
>> > ; error(io.error).
>> >
>> > + % maybe_incomplete_result is used for multibyte values read from a
>> > binary
>> > + % stream where it is possible for the final element in the stream
>> > to be
>> > + % incomplete.
>> > + %
>> > +:- type maybe_incomplete_result(T)
>> > + ---> ok(T)
>> > + ; eof
>> > + ; incomplete(list(uint8))
>> > + ; error(io.error).
>> > +
>>
>> I would add a sentence to explain what incomplete and its argument mean.
>
> Done.
>
>> > :- pred read_binary_uint8(io.binary_input_stream::in,
>> > io.result(uint8)::out,
>> > io::di, io::uo) is det.
>> >
>> > + % The following predicates read multibyte integer values from the
>> > current
>> > + % binary input stream or from the specified input stream.
>> > + %
>> > + % These names of these predicates have the form:
>>
>> *The* names of these predicates ...
>
> Fixed.
>
>> > + % read_binary_<TYPE><SUFFIX>
>> > + %
>> > + % where <TYPE> is the name of one of the Mercury multibyte fixed
>> > size
>> > + % integer types. <SUFFIX> is optional and specifies the byte order
>> > in
>> > + % which the integer value is read from the stream. It may be one
>> > of:
>>
>> "in which the integer value is read from the stream" is a bit ambiguous;
>> it can be read as "the byte order used in the input stream", which I think
>> is the meaning you intend, but it could also be read as "the byte order of
>> the machine doing the reading". I would reword to eliminate any
>> possibility
>> of misreading.
>
> Done.
>
>> > @@ -2132,6 +2256,19 @@ using System.Security.Principal;
>> > :- pragma foreign_export_enum("Java", result_code/0,
>> > [prefix("ML_RESULT_CODE_"), uppercase]).
>> >
>> > +:- type maybe_incomplete_result_code
>> > + ---> ok
>> > + ; eof
>> > + ; incomplete
>> > + ; error.
>>
>> These function symbol names are *seriously* overloaded. We can't easily do
>> anything about the ones in exported types, but I would add a
>> disambiguating prefix to these.
>
> I've added the prefix "mirc_" to them (and simplified their foreign names
> accordingly). (I'll add a prefix to the constructors of the existing
> result_code/0 type separately.)
>
>> > +%---------------------%
>> > +
>> > +read_binary_int16(Result, !IO) :-
>> > + binary_input_stream(Stream, !IO),
>> > + read_binary_int16(Stream, Result, !IO).
>> > +
>> > +read_binary_int16(Stream, Result, !IO) :-
>> > + ( if native_byte_order_is_big_endian then
>> > + read_binary_int16_be(Stream, Result, !IO)
>> > + else
>> > + read_binary_int16_le(Stream, Result, !IO)
>> > + ).
>> > +
>> > +:- pred native_byte_order_is_big_endian is semidet.
>> > +
>> > +:- pragma foreign_proc("C",
>> > + native_byte_order_is_big_endian,
>> > + [promise_pure, will_not_call_mercury, thread_safe],
>> > +"
>> > + #if defined(MR_BIG_ENDIAN)
>> > + SUCCESS_INDICATOR = MR_TRUE;
>> > + #else
>> > + SUCCESS_INDICATOR = MR_FALSE;
>> > + #endif
>> > +").
>> > +
>> > +:- pragma foreign_proc("C#",
>> > + native_byte_order_is_big_endian,
>> > + [promise_pure, will_not_call_mercury, thread_safe],
>> > +"
>> > + SUCCESS_INDICATOR = !(BitConverter.IsLittleEndian);
>> > +").
>> > +
>> > +:- pragma foreign_proc("Java",
>> > + native_byte_order_is_big_endian,
>> > + [promise_pure, will_not_call_mercury, thread_safe],
>> > +"
>> > + SUCCESS_INDICATOR =
>> > + (java.nio.ByteOrder.nativeOrder() ==
>> > java.nio.ByteOrder.BIG_ENDIAN);
>> > +").
>> > +
>> > +native_byte_order_is_big_endian :-
>> > + sorry($module,
>> > + "native_byte_order_is_big_endian/0 NYI for Erlang").
>>
>> Since the helper pred native_byte_order_is_big_endian is not specific
>> to reading in int16s, I would put its declaration and implementation
>> at the bottom of your additions, or at the bottom of the whole module.
>> You want the 16. 32 and 64 bit code to be as similar as possible to each
>> other; including this predicate that all of them need with one of them
>> is an unnecessary non-similarity.
>
> Done.
>
>> > +%---------------------%
>> > +
>> > +read_binary_int16_le(Result, !IO) :-
>> > + binary_input_stream(Stream, !IO),
>> > + read_binary_int16_le(Stream, Result, !IO).
>> > +
>> > +read_binary_int16_le(binary_input_stream(Stream), Result, !IO) :-
>> > + do_read_binary_uint16_le(Stream, Result0, UInt16, IncompleteBytes,
>> > + Error, !IO),
>> > + (
>> > + Result0 = ok,
>> > + Int16 = cast_from_uint16(UInt16),
>> > + Result = ok(Int16)
>> > + ;
>> > + Result0 = eof,
>> > + Result = eof
>> > + ;
>> > + Result0 = incomplete,
>> > + Result = incomplete(IncompleteBytes)
>> > + ;
>> > + Result0 = error,
>> > + make_err_msg(Error, "read failed: ", Msg),
>> > + Result = error(io_error(Msg))
>> > + ).
>>
>> We used to have code in the compiler where X0 and X were of different
>> types,
>> as here. I often found that they made the code harder to read, and fixed
>> that
>> by renaming. I would s/Result0/ResultCode/ here and in every other similar
>> situation in your diff.
>
> Done.
>
>> > +read_binary_int16_be(Result, !IO) :-
>> > + binary_input_stream(Stream, !IO),
>> > + read_binary_int16_be(Stream, Result, !IO).
>> > +
>> > +read_binary_int16_be(binary_input_stream(Stream), Result, !IO) :-
>> > + do_read_binary_uint16_le(Stream, Result0, UInt16LE,
>> > IncompleteBytes,
>> > + Error, !IO),
>> > + (
>> > + Result0 = ok,
>> > + UInt16BE = reverse_bytes(UInt16LE),
>> > + Int16 = cast_from_uint16(UInt16BE),
>> > + Result = ok(Int16)
>>
>> Doing the conversion from byte sequence to an N bit int one way, then
>> reversing the bytes in the N bit int, is slower than building the N bit
>> int
>> the right way in the first place. It does require fewer lines of code.
>> Is that your reason for choosing this approach?
>
> I've altered this below. For the C grade specifically, we are ultimately
> calling fread(), so the bytes will be read into the buffer in one direction
> (i.e. in some case we will need to reverse them).
>
>> > +:- pred do_read_binary_uint16_le(stream::in,
>> > maybe_incomplete_result_code::out,
>> > + uint16::out, list(uint8)::out, system_error::out, io::di, io::uo)
>> > is det.
>>
>> I presume the reason you return unsigned here is that the caller will
>> reshuffle the bytes, and keeping the data as unsigned until the last
>> moment
>> reduces the chance of bugs that cause sign extension to copy the most
>> significant bit of the wrong byte. I would add a comment to this
>> predicate,
>> and its mirrors for other values of N, saying this.
>
> No, it was simply to avoid inflating the number of foreign_procs; we have
> versions of reverse_bytes that work on signed integers so problems
> caused by sign extensions shouldn't be an issue for that operation.
>
>> > +:- pragma foreign_proc("C",
>> > + do_read_binary_uint16_le(Stream::in, Result::out, UInt16::out,
>> > + IncompleteBytes::out, Error::out, _IO0::di, _IO::uo),
>> > + [will_not_call_mercury, promise_pure, thread_safe,
>> > will_not_modify_trail],
>> > +"
>> > + unsigned char buffer[2];
>> > + size_t nread = MR_READ(*Stream, buffer, 2);
>> > + IncompleteBytes = MR_list_empty();
>> > +
>> > + if (nread < 2) {
>> > + UInt16 = 0;
>> > + if (MR_FERROR(*Stream)) {
>> > + Result = ML_MAYBE_INCOMPLETE_RESULT_CODE_ERROR,
>> > + Error = errno;
>> > + } else if (nread > 0) {
>> > + int i;
>> > + Result = ML_MAYBE_INCOMPLETE_RESULT_CODE_INCOMPLETE;
>> > + IncompleteBytes = MR_list_cons(buffer[0], IncompleteBytes);
>> > + Error = 0;
>> > + } else {
>> > + Result = ML_MAYBE_INCOMPLETE_RESULT_CODE_EOF;
>> > + Error = 0;
>> > + }
>> > + } else {
>> > + Result = ML_MAYBE_INCOMPLETE_RESULT_CODE_OK;
>> > + #if defined(MR_BIG_ENDIAN)
>> > + ((unsigned char *) &UInt16)[0] = buffer[1];
>> > + ((unsigned char *) &UInt16)[1] = buffer[0];
>> > + #else
>> > + UInt16 = *((uint16_t *) buffer);
>> > + #endif
>> > + Error = 0;
>> > + }
>> > +").
>>
>> Both paths through the code for the OK case work only on machines with
>> 8 bit bytes. That is ok, since we only support those machines.
>
> GCC and clang both only support machines with 8-bit bytes, nevermind us ;-)
> (MSVC is almost certainly the same.)
>
>> However, the code of the MR_BIG_ENDIAN case should be replaced with
>> a call to the existing macro MR_uint16_reverse_bytes.
>
> I forgot it was there!
>
>> Not only does it work on non-8-bit-byte machines, but it should be faster,
>> since it does not require any load or store operations.
>
> It definitely won't work on non-8-bit-byte machines in some cases since the
> fallback definitions it uses in the absence of any C compiler intrinsic
> functions *do* assume 8-bit bytes.
>
>> The code would also be shorter, and would explicitly document your intent.
>
> Done.
>
>> Since your approach makes e.g. read_uint16_be use
>> do_read_binary_uint16_le,
>> do_read_binary_uint16_le should defined not just after read_uint16_le
>> but also after read_uint16_be as well.
>
> I've changed this so that read_binary_uint16_{be,le} both call
> do_read_binary_uint16 and that as an extra argument specifying the endianess
> of
> the values in the stream. Similarly for the other sizes and signednesses.
>
>> What I would do write for C is a pragma foreign_code that
>>
>> - defines a macro for building big endian N byte uints from an N element
>> array of bytes, where N is 2, 4 or 8;
>>
>> - defines a little endian version of that same macro,
>>
>> - defines a macro for the body of do_read_binary_uintN_EN that invokes
>> placeholder macros (named maybe ML_build_int) for the OK case.
>>
>> Then the definition of each of do_read_binary_uintN_EN would just plug
>> one of the first two sets of macros into an invocation of the third.
>
> Done.
>
>> The runtime uses this technique extensively to minimize unnecessary code
>> duplication when defining multiple related pieces of code. I don't know
>> whether C# or Java have any equivalent mechanism.
>
> There's no preprocessor or templating if that's what you mean; there
> are other approachs that might work but they would be less efficient.
>
>> > + UInt32 = (uint) (buffer[3] << 24 | buffer[2] << 16 |
>> > + buffer[1] << 8 | buffer[0]);
>>
>> > + UInt32 =
>> > + (buffer[3] & 0xff) << 24 |
>> > + (buffer[2] & 0xff) << 16 |
>> > + (buffer[1] & 0xff) << 8 |
>> > + (buffer[0] & 0xff);
>> > + }
>>
>> Any reason why the C# and Java versions are laid out differently?
>
> No, I've adjusted them to be consistent.
>
>> Any reason why the C code uses a completely different approach?
>
> The C versions are taking advantage of the fact that in some cases we can
> just
> read the integer value directly from the byte buffer. Doing the same in the
> other languages is more expensive that what we do here.
>
>> > diff --git a/tests/hard_coded/read_binary_int16.exp
>> > b/tests/hard_coded/read_binary_int16.exp
>> > index e69de29..c54276a 100644
>> > --- a/tests/hard_coded/read_binary_int16.exp
>> > +++ b/tests/hard_coded/read_binary_int16.exp
>
> ...
>
>> I would restructure the tests so that each input is reported on only once,
>> like this:
>>
>> Input: [1u8, 0u8] (LE: 1) (BE: 256)
>> Big endian result: 256i16
>> Little endian result: 1i16
>> Native result: 1i16
>>
>> It would also make the tests faster, since each test file contents would
>> be
>> created just once, not three times.
>
> I'll do that as a separate change.
>
> ...
>
>> > + io.remove_file(test_file, _, !IO)
>> > + ;
>> > + OpenInResult = error(IO_Error),
>> > + io.format("I/O ERROR: %s\n",
>> > [s(io.error_message(IO_Error))],
>> > + !IO)
>> > + )
>> > + ;
>> > + OpenOutResult = error(IO_Error),
>> > + io.format("I/O ERROR: %s\n", [s(io.error_message(IO_Error))],
>> > !IO)
>> > + ).
>>
>> This code closes the binary output file, but not the binary input file.
>>
>> The same comments apply to the other test cases as well.
>
> Fixed.
>
> Julien.
>
More information about the reviews
mailing list