Project

General

Profile

Actions

Feature #473

closed

Network protocol: 8bit and 16bit delta array lengths

Added by Marko Lindqvist 7 months ago. Updated 7 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Bootstrap
Target version:
Start date:
04/19/2024
Due date:
% Done:

0%

Estimated time:

Description

Recently delta protocol array indices were bumped from 8bit to 16bit, to support arrays with more elements. However, this means that transferring any array headers now take twice as much space.

Maybe we should have two array types? One with 8bit indices, and one with 16bit indices. E.g., "uint short_array[less_than_254_elems]8;" & "uint long_array[more_than_253_elems]16;" Or maybe have the 8bit array the default, and make an array 16bit one only by specific marking?


Files

Actions #1

Updated by Alina Lenk 7 months ago

  • Category set to Bootstrap

Has this had an actual noticeable impact, or are we prematurely optimizing? As much as it pains me to admit it, there does come a point where performance optimization might not be worth making the code more complex. Most of our network traffic is compressed, so I'd hope if there's enough extra null bytes to be a significant concern, compression would mitigate that again.

If we're gonna do this, I'd prefer not to complicate the packets.def format; and more importantly, not to specify the index size limit in two places, so whenever we change a constant used as an array's declared/maximum size, we also have to check whether we now have to change a bunch of index sizes.
We could determine the required index type statically by comparing the declared max size to MAX_UINT8; or we could even do it dynamically (if there were any diff arrays with non-constant size), since at the time we send or receive a deep-diff array, we already know the length of that array, and thus the max index. Though doing it dynamically would make the generated code a lot more complex.
(If and when we're gonna be deep-diffing vectors, we'll only have the dynamic option, naturally.)

Actions #2

Updated by Alina Lenk 7 months ago

Alina Lenk wrote in #note-1:

Though doing it dynamically would make the generated code a lot more complex.

Turns out after some testing, the simpler way I thought would work in the static case doesn't actually work without extra macro ugliness. So if we do this it's gonna result in a bunch of if-elses (rather than just a single one at the start) either way; just in the static case, they're preprocessor if-elses rather than runtime ones. I've also realized we can simplify some of the logic by using the actual array size as a sentinel value, rather than MAX_UINT8/MAX_UINT16.

Actions #3

Updated by Alina Lenk 7 months ago

Voila. I did reformat and move around some parts of it to make that part of the code more readable, I think it's okay now.

Actions #4

Updated by Marko Lindqvist 7 months ago

Should we have this also in S3_2, as the original change to 16bit array lengths was applied there?

Actions #5

Updated by Alina Lenk 7 months ago

Marko Lindqvist wrote in #note-4:

Should we have this also in S3_2, as the original change to 16bit array lengths was applied there?

Probably, yeah.

Actions #8

Updated by Alina Lenk 7 months ago

  • Status changed from In Review to Closed
Actions

Also available in: Atom PDF