Für vnsiserver bitte wieder default verwenden
Mein fork darf in Rente gehen, danke an M-Reimer für das mergen der letzten Änderung.
Da es jetzt geht, kann man den "Angstpuffer" aus dem responspacket auch rausschmeissen.
Den neu angeforderten Speicherblock habe ich auch etwas verkleinert. 50Byte sollten für ~2-3Kanäle reichen, man muss es ja nicht übertreiben.
Ich finde diese deine Idee gut, auch ohne dringende Notwendigkeit. Könnte das nicht einfach bleiben?
Ich hab immer noch nicht ganz verstanden wo der Puffer genutzt wird. Wenn der binär übers Netzwerk geht wäre schon sinnvoll Platz zu sparen.
Die 80Byte sind schlicht leerer, unbenutzter Raum am Ende des reservierten Speicherbereichs, schaden tut es also nicht.
Das hatte ich eingefügt, falls da mit der Berechnung was schief geht und jemand deswegen "hinten" über diesen Bereich hinaus schreibt.
Der Wert war extremst großzügig gewählt, um alle Eventualitäten abzudecken. Inzwischen bin ich aber ziemlich sicher, dass die Berechnungen stimmen. Des weiteren ist mir aber klar geworden, dass man eigentlich nie mehr als ein Byte falsch liegen kann, da sonst Fehler ("\0" abgeschnitten, fehlende Werte, ungültige Kanalbeschreibung, ...) in dem Responspacket entstehen würden und die würden auffallen. (Ausnahme: strlen() liefert einen falschen Wert, da hätte man dann aber eh ein Problem.)
Ausserdem wird auch der EPG mit diesem Responspacket übertragen und da gab es nie Ärger.
Die Blockgröße in der der Speicher geholt wurde, war für die Kanalliste auch eher großzügig gewählt (~20 Kanäle).
Beim EPG bin ich nicht sicher, wie groß das wird.
Man kann das auch auf den 512Byte lassen, bei EPG könnte das sogar Sinn machen.
Diesen 80Byte-Puffer würde ich aber auf 1 reduzieren, oder zumindest einen Hinweis einfügen. Sonst steigt der Nächste da nie durch.
Jetzt hab ich mich notgedrungen doch mal mit der Datei responsepacket.c auseinandergesetzt. Im Prinzip wäre es nicht verkehrt das komplett neu zu schreiben. So einen dynamischen Buffer baut man am besten mit einem "std::string", der einem sowas wie die Speicherverwaltung direkt abnimmt.
Erstmal zum Thema "sicherheitshalber etwas früher mehr Speicher holen" (ein Byte mehr für vergessenes "trailing zero"), also diese Zeile wo im unverändeten Code stand:
if ((bufUsed + by) < bufSize)
{
return;
}Soll heißen: Wenn "Füllstand plus nötige Bytes" KLEINER IST als die mögliche Größe des Buffers, dann nichts tun. Hier dürfte in Theorie auch "<=" stehen, denn wenn der Buffer genau die größe hat, die gebraucht wird, dann könnte man den ja einfach so verwenden. Hier ist also, wenn ich das nicht falsch verstanden habe, bereits ein "Sicherheits-Byte" vorgesehen.
==> Ich würde diese Stelle einfach "im Original" verwenden.
Wegen der "growth strategy" bin ich mal in der Historie etwas zurück und hab dort das gefunden: https://github.com/vdr-project…91ad8830af8ce970de9f4L277
if (512 > by) by = 512;Das könnte man frei interpretieren als "hole mindestens immer 512 Bytes Speicher".
Ich würde vorschlagen diese Zeile einfach wieder rein zu nehmen. Die ist beim dort stattgefundenen Maintainer-Wechsel und dessen Überarbeitung "untergegangen", war aber vom ursprünglichen Entwickler sicher nicht umsonst so gewählt.
Ich würde das aber rein gefühlsmäßig umdrehen in:
if (by < 512) by = 512;Das etwas mehr aufmal reservieren bedeutet vor allem weniger warten auf langsames malloc.
==> Ich würde diese Stelle einfach "im Original" verwenden.
Ich wollte eigentlich wieder dahin zurück gehen, hat ja funktioniert.
Ich würde vorschlagen diese Zeile einfach wieder rein zu nehmen.
Oder Sowas:
void cResponsePacket::checkExtend(uint32_t by)
{
if ((bufUsed + by) < bufSize )
return;
by = 512 * ((by / 512) + 1);
uint8_t* newBuf = (uint8_t*)realloc(buffer, bufSize + by);
...Vorteil wäre, dass man immer vielfache von 512 bekommt, das freut die Speicherverwaltung.
Wobei das in diesem Fall wahrscheinlich nicht wirklich was Ausmacht, da das Objekt (evtl. sogar der ganze Thread?) nach erfolgter Übertragung eingestampft wird.
Deinen zweiten Vorschlag hatte ich auch erst vorbereitet aber mir dann gedacht das ja eigentlich total wumpe ist in welchem Raster man Speicher abfragt. Und ja, das verfällt alles wenn das Paket raus ist.
Das ResponsePacket-Objekt verfällt, das ist sicher.
Wenn der Thread weiter lebt und dann das nächste Responsepacket fürs EPG baut, könnte es was bringen, einen Raster einzuhalten. - Zumindest theoretisch.
Was mit dem Thread passiert, habe ich mir aber nicht näher angesehen.
Im Prinzip gehört die gesamte "cResponsePacket" komplett neu geschrieben. Mal sehen ob ich da irgendwann mal Zeit und Lust für habe.
Statt mehrerer "init..." und "finalize..." wäre sinnvoll eine Basisklasse zu haben und dann die Varianten davon abzuleiten. Allerdings müsste ich dafür erstmal komplett erfassen wie "cResponsePacket" genutzt wird. Bisher sieht es so aus als würde immer nur ein Paket erstellt und das Objekt dann verfallen gelassen. Ein "Wiederverwenden" eines Buffers findet scheinbar nicht statt. In dem Fall könnte man die "init..." auch als Konstruktor der abgeleiteten Klassen anlegen.
Auf jeden Fall hab ich jetzt erstmal die "Realloc Strategie" des ursprünglichen Autors wieder hergestellt und ein paar Kommentare hinzugefügt. Wenn damit alles läuft würde ich das die Tage mal als neues Release markieren.
Die datei HISTORY muss vorher aktualisiert werden.
"cResponsePacket" baut, soweit ich gesehen habe, lediglich diese Pakete und wird wohl immer nur einmal benutzt.
Die mehreren "init..." und "finalize..." kommen daher dass sich die Header der Pakete, je nach Inhalt, unterscheiden.
Das könnte man wohl recht einfach in eine Basisklasse und abgeleitete "cEPGResponsePacket", "cChannelsResponsePacket".... Pakete umbauen, wenn man will.
Dann wäre etwas intern auf Basis std::stringstream ein guter Ersatz.
Ich habe hier mal einen Vorschlag abgelegt, wie man cResponsePacket überarbeiten könnte.
Die grobe Idee ist
- es gibt kein cResponsePacket::init* mehr, dafür aber dann drei Konstruktoren, je nach Art von response.
init* taucht zur Zeit nach jedem neuen cResponsePacket auf, das macht keinen Sinn.
- cResponsePacket::finalise() wird private member der Klasse, verschwindet im sonstigen code.
finalise() wird zur Zeit jedesmal aufgerufen, bevor die fertigen Daten verwendet werden. Unmittelbar danach wird das Objekt zerstört.
Warum dann nicht das einfach intern aufrufen.
- die neue Version soll binär kompatibel die Daten übertragen - 1:1 gleiche Bytes.
Das macht das debuggen wirklich einfacher.
- die Speicherverwaltung übernimmt neu dann std::vector<uint8_t>, keine lokalen Baustellen mit Speicher mehr in der Klasse.
- es gibt einen neuen Ordner /tests im sourcecode, der solche tiefen Eingriffe im Plugin isoliert testen lässt.
- Ausführungsgeschwindigkeit etwa mit der alten Klasse vergleichbar. Siehe neuer Ordner tests, da gibt es dafür ein Programm.
Aber wenn(!) man diese Klasse cResponsePacket überarbeiten würde, dann müsste man das Gegenstück, cRequestpacket ebenso anfassen müssen.
Was denkt ihr? Weiter verfolgen oder verwerfen?
Ist noch nicht am lebenden Objekt getestet.
Ist jetzt für mich spontan nicht ganz zu erfassen. Müsste ich mich mal länger mit befassen. Ich weiß z.B. das es Templates gibt, hab die aber selbst nie verwendet.
Für meinen Geschmack ist da noch zu viel "Low-Level Speicher Kram" drin. Hätte etwas dagegen gesprochen als Buffer einen "std::string" zu nutzen? Und dann mit "append" einfach alles hinten anhängen? Eventuell wäre das sogar (je nachdem wie da intern optimiert wird) effizienter weil Daten blockweise kopiert werden statt Byte für Byte in einer Schleife.
Und eventuell doch eine Basis-Klasse und dann abgeleitete Klassen pro "Paket-Typ"? Hätte dann den Vorteil das jede Klasse ein eigenes "finalize" haben kann. Oder doch ein globales "finalize" und als Konstanten im "Private" der jeweiligen Klasse dann die relevanten Positionen.
Gegen std::string spricht eindeutig, dass der Inhalt der Daten mit voller Absicht auch Nullen '\0' enthält. Die sind normaler Teil der Datenpakete.
std::string ist nun einmal eher ein Container für nullterminierte const char* und beendet dann auch Daten bei Nullen.
std::string ist klasse, aber eben nicht hier. Falsche Anwendung, das sind keine strings, sondern bytes.
std::stringstream stolpert sowohl über über die Nullen im Datenpaket, als auch über die Tatsache, dass erst nachträglich noch einige bytes am Anfang des Streams geändert werden müssen. Genauer, die Anzahl der Bytes, die dem Header folgen, als uint32_t.
std::vector ist all dem gewachsen, linearer Speicher, die Anzahl member (hier bytes) ist in der standard lib verwaltet.
Und beliebiger Zugriff nachträglich möglich auf bytes mitten drin..
Was die Effizienz betrifft, laut Testprogramm in ./test ist der neue Code geringfügig(!) schneller als der alte. Zumindest mal nicht schlechter.
Aber ja, das ist eine Diskussionsgrundlage. Nicht mehr.
Nicht dass meine Meinung ins Gewicht fällt (ich benutze das Plugin nicht mal), aber der Vorschlag gefällt mir.
Viel übersichtlicher und vor allen Dingen ist man die fehleranfällige Zählerei endlich los.
Das Paket ist nun mal eine bunte Mischung aus diversen Daten, da kann man nichts machen (es sei denn, man will das alles umbauen).
Allein bei bei einem Kanal gibt es mehrere Integer und Strings, schön durcheinander.
Die Reihenfolge ist aber vorgegeben und muss eingehalten werden und fehlen darf wohl auch nichts. Auch der Typ der Integer-Werte muss exakt passen, sonst gibt's Chaos.
Man könnte jetzt einen Kanal als Structure oder sonst wie definieren und dann als Ganzes dem responsepacket hinzufügen, das wäre evtl. übersichtlicher. Allerdings müsste man das dann auch fürs EPG usw. machen.
Am Ende viel Arbeit, dafür dass man es jeweils nur an einer Stelle nutzt.
Und man müsste genau verstehen wie jeder Pakettyp aufgebaut ist, nicht nur wie ein Pakt aus den Komponenten zusammengebaut wird.
Und eventuell doch eine Basis-Klasse und dann abgeleitete Klassen pro "Paket-Typ"?
Daran hatte ich auch zuerst gedacht. Der aktuelle Vorschlag gefällt mir aber eigentlich besser, da einfacher.
Bei finalize() macht nur die Headerlänge eine Unterschied, das Problem ist recht einfach zu beseitigen.
In meinem aktuellsten Vorschlag (bislang ungetestet!) bin ich sogar die hart codierten Headerlängen los geworden.
Allerdings geht das reserve() so nicht mehr, das dürfte aber so viel nicht ausmachen.
Die Alternative mit reserve() ist auskommentiert aber auch noch drin.
- die neue Version soll binär kompatibel die Daten übertragen - 1:1 gleiche Bytes.
Das macht das debuggen wirklich einfacher.
Da würde ich auch nicht dran drehen, allein um die Kompatibilität mit den Clients zu erhalten.
Aber wenn(!) man diese Klasse cResponsePacket überarbeiten würde, dann müsste man das Gegenstück, cRequestpacket ebenso anfassen müssen.
cRequestpacket hat aber zumindest nicht die Problematik mit der Speicherverwaltung.
Für mich sieht es eher lediglich nach einem Parser aus.
Das könnte man sicher optimieren, wenn man will, wäre für mich aber nicht zwingend.
Das gleiche gilt auch für die add_*() Funktionen, die man durch eine universelle (überladene) add() ersetzen könnte.
Da man den genauen Typ den Wertes aber kennen und einhalten muss, hat man nicht viel dadurch gewonnen. Evtl. richtet man damit sogar Schaden an, wenn die Ausgangsdaten nicht passen.
Die Variable header_size wäre am besten ein size_t statt uint32_t. reserve() darf gefahrlos bleiben bei deiner Änderung, das ist nur eine Optimierung, damit nicht Speicher in kleinen Häppchen angefordert wird.
Quoteeine universelle (überladene) add()
wäre immer eine Stolperstelle. z.B. wäre ein add(0) mehrdeutig, ist das nun int, uint32_t, int32_t, ..?
Die Variable header_size wäre am besten ein size_t statt uint32_t.
Danke, das hätte auch ein size_t werden sollen.
Da habe ich wohl die falsche Zeile als Vorlage kopiert 
.
reserve() darf gefahrlos bleiben bei deiner Änderung, das ist nur eine Optimierung, damit nicht Speicher in kleinen Häppchen angefordert wird.
Ich hatte es nur raus genommen, da ich dort die Header-Länge noch nicht weiß und diese hart codierten Zahlen los werden wollte.
Da hatte ich gestern aber zu kompliziert gedacht, das Paket wird ja eh deutlich größer. Da kann man auch etwas mehr reservieren, wie früher auch schon.
Ich bin jetzt mal der Vorgabe des Original-Autors gefolgt und auf 512 Byte gegangen, das kann man aber noch anpassen.
Das Paket mit der Kanallist wird, bei ~30Byte pro Kanal, ziemlich sicher größer.
Das mit dem EPG wahrscheinlich auch. Beim Rest kann ich das nicht abschätzen.
Aber mindestens 100Byte zu reservieren dürfte nicht zu viel Speicher verschwenden. Mann kann ja jetzt auch mal schauen, wie sich das auf die Geschwindigkeit auswirkt.
wäre immer eine Stolperstelle. z.B. wäre ein add(0) mehrdeutig, ist das nun int, uint32_t, int32_t, ..?
Etwas in der Art meinte ich mit "evtl. richtet man damit sogar Schaden an".
Das Problem ist, dass genau an dieser Stelle der Aufbau von dem Paket definiert wird. Wenn man das irgendwo anders vorher definiert hätte wäre das sicher übersichtlicher.
Mehr abstrahieren wird also schwierig, so wie das Plugin momentan aufgebaut ist.
Ich denke, der aktuelle Vorschlag ist schon ziemlich das Optimum, was man beim cResponsePacket raus holen kann, ohne das Plugin komplett umzubauen. Allein dass die fehleranfällige Rechnerei weg fällt, lohnt sich IMHO.
Don’t have an account yet? Register yourself now and be a part of our community!
