Ein paar Gedanken zu `for`, `goto`, Lesbarkeit

2019-12-29

nullprogram.com ist eines der besten Blogs, die ich kenne. Quasi jeder Artikel ist interessant und ich lerne etwas neues.

Heute hat er einen Artikel zu purgeable memory veröffentlicht. Kurze Zusammenfassung des Konzepts: Es geht um Speicher, der vom Kernel freigegeben (nicht geswappt!) werden kann, falls es die globale Arbeitslast im System erfordert. Der eigene Code muss damit natürlich umgehen können.

Die Idee dahinter ist: Eine der Berechnungen, die der Code durchführt, dauert eine Weile und nimmt spürbar Speicher in Anspruch. Das Ergebnis will man mehrfach verwenden, zwischen den Verwendungen liegen aber größere zeitliche Pausen. Außerdem ist es schneller, die Berechnung einfach nochmal durchzuführen, als auf den Kernel zu warten, bis der irgendwas aus dem Swap wieder rausgeholt hat.

Als Beispiel gibt es diesen Codeschnipsel:

/* Version 0: Original code */

uint32_t *texture = 0;
struct png *png = png_load("texture.png");
if (!png) die();

/* ... */

for (;;) {
    if (!texture) {
        texture = purgeable_alloc(png->width * png->height * 4);
        if (!texture) die();
        png_decode_rgba(png, texture);
    } else if (!purgeable_lock(texture)) {
        purgeable_free(texture);
        texture = 0;
        continue;
    }
    glTexImage2D(
        GL_TEXTURE_2D, 0,
        GL_RGBA, png->width, png->height, 0,
        GL_RGBA, GL_UNSIGNED_BYTE, texture
    );
    purgeable_unlock(texture);
    break;
}

Eine Textur wird geladen und in einem OpenGL-Kontext verwendet. texture ist also eine langlebige Variable. Man denke beispielsweise an ein Menü in einem Spiel: Das Menü wird nur ab und zu mal aufgerufen und zeigt immer dasselbe Hintergrundbild – unsere texture. Das Menü soll sich schnell öffnen, also würde man die nötigen Daten gerne im Speicher behalten. Auf der anderen Seite: Wenn das Menü nur selten aufgerufen wird, muss das dann wirklich im Speicher bleiben? Wäre nett, zwingend notwendig ist es aber eben nicht. Also könnte man purgeable memory dafür verwenden.

Mir geht es jetzt um die for-Schleife da oben.

Die hat mich am Anfang ziemlich verwirrt. Warum ist da eine Schleife? Warum ist sie endlos? Müssen wir da vielleicht etwas mehrfach probieren, weil es fehlschlagen könnte? Einen Moment später sind mir dann continue und break aufgefallen und dann hat’s geklingelt.

Was eigentlich passiert, ist das (Pseudocode):

if (texture not loaded)
    load texture;
else /* texture was once loaded */
    if (kernel has purged the memory in the meantime)
        mark texture as "not loaded";
        TRY AGAIN FROM THE TOP;

use texture in OpenGL;
mark texture memory as purgeable;

Mit anderen Worten, die for-Schleife wird benutzt, um ein goto zu vermeiden.

Ein Vorteil dabei ist, dass die geschweiften Klammern unmissverständlich klarmachen, wo dieser Abschnitt beginnt und endet. Es kann keine unerwarteten Sprünge von woanders im Code geben, also wird eine der größten Fallen von goto ausgeschlossen.

Auf der anderen Seite habe ich den Eindruck, dass dieses Konstrukt nicht unbedingt „besseren“ Code erzeugt. Man muss es zweimal lesen und es ist auch gar keine Schleife. Es ist eher ein „probier’s nochmal“-Mechanismus.

Wenn man’s mal verstanden hat, ist es trivial. Es ist aber eines dieser Dinge, die so eine kleine „Barriere“ erzeugen. Wenn solche Tricks an sehr vielen Stellen benutzt werden, wird der Code unzugänglicher und es kann auch recht schwer für neue Leute im Team werden. Ich bin da natürlich keine Ausnahme und mache sowas auch. Klar, wenn man Code selbst schreibt, versteht man immer alles und alles ist trivial. Man vergisst zu leicht, dass man selbst auch irgendwann wieder „neu im Team“ ist, einfach weil man nach ein paar Jahren, in denen man den Code nicht angefasst hat, diese Tricks auch alle wieder vergessen hat. Das ist jetzt keine neue Erkenntnis, aber sie gerät eben allzu leicht in Vergessenheit.

Mit einem goto sähe es so aus:

/* Version 1: goto */

load_texture:
    if (!texture) {
        texture = purgeable_alloc(png->width * png->height * 4);
        if (!texture) die();
        png_decode_rgba(png, texture);
    } else if (!purgeable_lock(texture)) {
        purgeable_free(texture);
        texture = 0;
        goto load_texture;
    }
    glTexImage2D(
        GL_TEXTURE_2D, 0,
        GL_RGBA, png->width, png->height, 0,
        GL_RGBA, GL_UNSIGNED_BYTE, texture
    );
    purgeable_unlock(texture);

Ist das besser? Ich denke schon. Es gibt keine Verwirrung über eine Schleife mehr. Die unzutreffenden Vokabeln continue und break sind weg. Man liest das Label zuerst, ist also gewahr, dass da ein goto kommen wird. Label und goto sind auch recht nah beieinander. In meinen Augen ist das durchaus besser verständlich.

Ich benutze goto ab und zu, bin aber immer vorsichtig. Es muss einen guten Grund dafür geben. Es muss dabei helfen, Code zu schreiben, der besser verständlich ist. In diesem Fall hier wäre das in meinen Augen gegeben.

Die offensichtliche Frage ist aber, ob wir den Sprung überhaupt brauchen. Wie wäre es damit:

/* Version 2: Restructured code */

if (texture && !purgeable_lock(texture)) {
    purgeable_free(texture);
    texture = 0;
}

if (!texture) {
    texture = purgeable_alloc(png->width * png->height * 4);
    if (!texture) die();
    png_decode_rgba(png, texture);
}
glTexImage2D(
    GL_TEXTURE_2D, 0,
    GL_RGBA, png->width, png->height, 0,
    GL_RGBA, GL_UNSIGNED_BYTE, texture
);
purgeable_unlock(texture);

Der Code wurde umstrukturiert und es gibt jetzt kein „probier’s nochmal“ mehr. Der Pseudocode dazu sieht so aus:

if (kernel has purged our memory)
    reset pointer;

if (texture not loaded)
    load texture;
use texture in OpenGL;
mark texture memory as purgeable;

Ein bisschen hässlich ist, dass der Check für texture == NULL dupliziert ist. Es ist außerdem wichtig, dass man es nicht als else if schreibt, weswegen ich die leere Zeile drinhabe.

Geht es noch prägnanter? Was wir ja eigentlich machen wollen, sieht so aus:

if (texture not available)
    load texture;
use texture in OpenGL;
mark texture memory as purgeable;

Es gibt keine Schleife, keinen Sprung, nichts. Die Absicht ist klar formuliert. Kriegen wir das hin? Im Moment nicht. Nehmen wir aber mal an, die API der Bibliothek würde sich leicht ändern:

NULL-Pointer unterstützen: purgeable_lock() würde einfach False oder 0 zurückgeben.
Bei fehlgeschlagenen Locking-Versuchen automatisch den Speicher freigeben (wie im ursprünglichen Artikel vorgeschlagen).

Dann könnte man das machen:

/* Version 3: Altered API */

if (!purgeable_lock(texture)) {
    texture = purgeable_alloc(png->width * png->height * 4);
    if (!texture) die();
    png_decode_rgba(png, texture);
}
glTexImage2D(
    GL_TEXTURE_2D, 0,
    GL_RGBA, png->width, png->height, 0,
    GL_RGBA, GL_UNSIGNED_BYTE, texture
);
purgeable_unlock(texture);

Das ist die knappste und prägnanteste Variante, die ich finden konnte.

Auf der anderen Seite ist das wieder eine Grauzone. In Variante 3 muss man nämlich mehr mitdenken:

Falls texture == NULL ist, tut die Bibliothek nichts und returned False. Wir müssen jetzt das PNG decoden.
Falls texture != NULL ist:
- Locking hat geklappt? Alles gut, war ja schon decoded.
- Locking fehlschlagen? Dann müssen wir das PNG erneut decoden.

Je älter ich werde, desto mehr stört mich sowas. Es ist nur ein Funktionsaufruf, ich muss aber mehrere Situationen und Codepfade im Kopf durchsimulieren. Wenn man nicht aufpasst, vergisst man den Fall mit NULL vielleicht sogar. Selbst, wenn wir die Bibliothek also wie oben geändert hätten, würde ich den Code eher so schreiben:

/* Version 4: Altered API and explicit check */

if (!texture || !purgeable_lock(texture)) {
    texture = purgeable_alloc(png->width * png->height * 4);
    if (!texture) die();
    png_decode_rgba(png, texture);
}
glTexImage2D(
    GL_TEXTURE_2D, 0,
    GL_RGBA, png->width, png->height, 0,
    GL_RGBA, GL_UNSIGNED_BYTE, texture
);
purgeable_unlock(texture);

Die Prüfung auf texture == NULL ist dabei nicht zwingend notwendig, weil das ja auch die Bibliothek für uns macht. Ich glaube trotzdem, dass es sinnvoll ist, das explizit auszuschreiben, weil der Leser jetzt sofort weiß, dass es bei diesem if um zwei verschiedene Fälle geht: Das PNG wird decoded, wenn es das allererste Mal ist oder falls der Speicher weggeworfen wurde.

Als Schlussbemerkung: Ich schreibe lieber texture == NULL statt !texture, aus genau denselben Gründen. Es ist ein Pointer und wir prüfen, ob er NULL ist. Länger, aber dafür glasklar.

Comments?

Ein paar Gedanken zu for, goto, Lesbarkeit

Ein paar Gedanken zu `for`, `goto`, Lesbarkeit