Optimize serialization format for 2 bytes ints #20120
Conversation
mypyc/lib-rt/librt_internal.c
Outdated
| if (likely(first != LONG_INT_TRAILER)) { | ||
| return _read_short_int(data, first); | ||
| } | ||
| // People who have literal ints not fitting in size_t should be punished :-) |
There was a problem hiding this comment.
This comment is out of date, since now anything not fitting in 29 bits triggers this path.
mypyc/lib-rt/librt_internal.c
Outdated
| one byte: last bit 0, 7 bits used | ||
| two bytes: last two bits 01, 14 bits used | ||
| four bytes: last three bits 011, 29 bits used | ||
| everything else: 00000111 followed by serialized string representation |
There was a problem hiding this comment.
What about using ...01111 (four 1 bits) for the final option, as this would leave ...0111 available for a possible 60-bit format in the future. (I don't have a strong opinion.)
There was a problem hiding this comment.
OK. TBH I doubt we will ever need this, but it is very easy to make this change now.
| return read_str_internal(data); | ||
| } | ||
|
|
||
| static inline char |
There was a problem hiding this comment.
Add comment explaining that this assumes that real_value is within allowed range (29 bits).
| _CHECK_READ(data, 3, CPY_INT_TAG) | ||
| // TODO: check if compilers emit optimal code for these two reads, and tweak if needed. | ||
| second = _READ(data, uint8_t) | ||
| two_more = _READ(data, uint16_t) |
There was a problem hiding this comment.
Since this code path will be quite rare, we could also read one byte at a time without any real performance impact. This would make this work the same on little and big endian systems.
There was a problem hiding this comment.
This will not help much supporting big-endian platforms as we always write short integers (i.e. 1, 2, or 4 bytes) as a single value. And changing this for e.g. 2 bytes will probably have some performance impact. I think we can safely postpone this until people will actually ask for big-endian support. And even then I would prefer to use some #ifdef way of supporting big-endian platforms.
There was a problem hiding this comment.
Btw this makes me think that right now this can produce some garbage on big-endian platforms without failing, which is not good. Also I found that there is #if PY_BIG_ENDIAN thing that we may use here.
There was a problem hiding this comment.
After all I decided to add the big-endian support, it is not that hard. Essentially the idea is that I simply conditionally add byte reverse whenever I read/write anything longer than 1 byte. I know this is not optimal for big-endian but it is conceptually easy to reason about.
I will also make a separate PR to handle floats more robustly.
| #define MAX_ONE_BYTE_INT 117 // 2 ** 7 - 1 - 10 | ||
| #define MIN_TWO_BYTES_INT -100 | ||
| #define MAX_TWO_BYTES_INT 16283 // 2 ** (8 + 6) - 1 - 100 | ||
| #define MIN_FOUR_BYTES_INT -10000 |
There was a problem hiding this comment.
I think we could extend the negative range further here (e.g. make it symmetric with the positive range), since whether the upper bound is ~512M or ~256M probably won't make much difference in practice.
There was a problem hiding this comment.
FWIW this will not really simplify code (and will actually make it less consistent). I would prefer to keep this as is, unless there is a reason to use 4 bytes storage for large negative integers.
| b = Buffer(b.getvalue()) | ||
| assert read_int(b) == i | ||
| for i in (-12345, -12344, -11, 118, 12344, 12345): | ||
| for i in (-100, -11, 118, 12344, 16283): |
There was a problem hiding this comment.
Test also each power of two up to, say, 200, just in case (positive and negative)? Maybe test both n**2 (lower bits all zero) and n**2 - 1 (lower bits all ones).
2 participants
This is important for serialized ASTs (think line numbers for every node). Also in this PR:
Buffertype.