Te veel van het „ontwerp" van Darktable is begonnen met „het zou cool zijn als we … konden". Ik zal je vertellen wat cool is: zo snel mogelijk goede foto’s van jezelf aan je muren hangen. Beeldende kunst is geen podiumkunst (zoals muziek of theater), dus alleen het resultaat telt. Alles wat daaraan voorafgaat is overhead, en dat wil je doorgaans minimaal houden. Dat wil niet zeggen dat je niet van het proces zelf kunt genieten. Maar om van het proces te genieten, moet je je gereedschap beheersen en het naar jouw wil buigen, anders vecht je er alleen maar tegen en komt het hele proces neer op frustratie. Het probleem is dat het „ontwerp" van Darktable te veel moeite steekt in anders zijn omwille van het anders zijn.
In dit proces van het toevoegen van „coole nieuwe dingen" heeft Darktable sneltoetsen en veel basaal GUI-gedrag kapotgemaakt, waarbij schone code werd vervangen door spaghetti en er meer GUI-rommel werd toegevoegd zonder ooit iets te snoeien.
Ansel heeft een expliciet ontwerpproces dat verplicht begint met gedefinieerde problemen die worden ondervonden door gedefinieerde gebruikers. Het blijkt dat de hoeveelheid code die je moet schrijven omgekeerd evenredig is met de hoeveelheid nadenken die je over je oplossing hebt gedaan, doorgaans om het onderliggende probleem te ontdekken uit wat gebruikers je vertellen, en om de eenvoudigste weg naar een oplossing te vinden (die vaak niet eens een softwareoplossing is…).
Maar bugs wachten niet op je in het nadenken, ze wachten alleen in de code die je hebt geschreven. Dus hoe meer je nadenkt, hoe minder je codeert, hoe minder onderhoudslast je voor jezelf in de toekomst creëert. Maar natuurlijk… moet je genoeg tijd hebben om dingen goed te doordenken. In wezen betekent dat: dag dag zaterdagmiddag-, amateurgedreven gehack!
Breid het niet uit als je het niet eerst kunt vereenvoudigen
Veel van het gehack aan Darktable is gedaan door code te kopiëren en te plakken, uit andere delen van de software, of zelfs uit andere projecten, vooral omdat bijdragers niet de tijd noch de vaardigheden hebben om grote herschrijvingen te ondernemen. Dit veroorzaakt codeduplicatie en vergroot de lengte van functies, voegt interne vertakking toe en introduceert if en switch case die soms op meer dan 4 niveaus genest zijn, waardoor de structuur en logica moeilijker te bevatten zijn en bugs moeilijker (en frustrerender) op te sporen, terwijl ze waarschijnlijker voorkomen.
In elk geval, wanneer de code die verantwoordelijk is voor bestaande functies alleen maar groeit (soms met een factor 10 over 4 jaar), roept dat serieuze vragen op met betrekking tot toekomstige onderhoudbaarheid, in een context waarin bijdragers niet langer dan een paar jaar blijven hangen, en ontwikkelaars beperkt de tijd hebben om te investeren. Het is gewoonweg onverantwoordelijk, aangezien het onderhoudbaarheid op lange termijn opoffert voor glimmende nieuwe dingen.
Code vereenvoudigen en generaliseren, via schone API’s, voordat je nieuwe functies toevoegt is een must, en Ansel accepteert alleen code die ik persoonlijk begrijp en die ik de vaardigheden heb om te onderhouden. KISS.
Basale codeerlogica
Pull requests die niet voldoen aan de minimale eisen voor codekwaliteit worden niet geaccepteerd. Deze eisen zijn bedoeld om onderhoudbaarheid en stabiliteit op lange termijn te waarborgen door duidelijke, leesbare code af te dwingen die met een eenvoudige logica is gestructureerd.
Procedures moeten waar mogelijk worden opgesplitst in eenheids-, herbruikbare functies. Een uitzondering hierop zijn gespecialiseerde lineaire procedures (geen vertakking) die taken uitvoeren die te specifiek zijn om ergens hergebruikt te worden, maar gebruik in dat geval commentaar om de procedures op te delen in „hoofdstukken" of stappen die gemakkelijk te herkennen en te begrijpen zijn.
Functies moeten telkens slechts één taak uitvoeren. GUI-code mag bijvoorbeeld niet worden vermengd met SQL- of pixelverwerkingscode. Getters en setters moeten verschillende functies zijn.
Functies moeten slechts één ingangspunt en één uitgangspunt (return) hebben. De enige geaccepteerde uitzonderingen zijn een vroege return als de geheugenbuffer waarop de functie geacht wordt te opereren niet is geïnitialiseerd, of als een thread-mutexvergrendeling al is vastgezet.
Functies moeten leesbare, expliciete namen hebben en argumentnamen die hun doel kenbaar maken. Programma’s zijn bedoeld om door mensen te worden gelezen; als je voor de machine codeert, doe het dan in binair.
Functies mogen maximaal 2 if-voorwaardestructuren nesten. Als er meer dan 2 geneste if nodig zijn, moet de structuur van je code opnieuw worden geëvalueerd en waarschijnlijk worden opgedeeld in meer granulaire functies.
if mag alleen uniforme gevallen testen, zoals de toestand of de waarde van idealiter één (maar mogelijk meer) variabele(n) van hetzelfde type. Als niet-uniforme gevallen getest moeten worden (zoals IF user param IS value AND picture buffer IS initialized AND picture IS raw AND picture HAS embedded color profile AND color profile coeff[0] IS NOT NaN), moeten die worden uitbesteed aan een controlefunctie die een gbooleanTRUE of FALSE retourneert en die goed is genoemd, zodat collega-ontwikkelaars het doel van de controle begrijpen zonder ambiguïteit bij het vluchtig lezen van de code, zoals color_matrix_should_apply(). De vertakkingscode wordt dan if(color_matrix_should_apply()) pix_out = dot_product(pix_in, matrix);
Commentaar moet vermelden waarom je deed wat je deed, zoals je basisaannames, je redenen en elke academische of documentatiereferentie die je als basis hebt gebruikt (DOI’s en URL’s horen daar te staan). Je code moet expliciet vertellen wat je hebt gedaan. Als je merkt dat je in commentaar moet uitleggen wat je code doet, is dat meestal een teken dat je code slecht is gestructureerd, dat variabelen en functies slecht zijn benoemd, enzovoort.
Snelle workarounds die problemen verbergen in plaats van ze bij de wortel aan te pakken, worden niet geaccepteerd. Als je daarin geïnteresseerd bent, overweeg dan om in plaats daarvan bij te dragen aan het upstream darktable. De enige uitzonderingen zijn wanneer de problemen blokkerend zijn (de software laten crashen) en er na een behoorlijke hoeveelheid tijd besteed aan onderzoek geen betere oplossing is gevonden.
Onthoud altijd dat de beste code de eenvoudigste is. KISS. Om dit doel te bereiken is het meestal beter om code vanaf nul te schrijven dan te proberen stukjes bestaande code aan elkaar te knutselen door veel te kopiëren en te plakken.
Enkele willekeurige stukjes wijsheid van het internet:
Iedereen weet dat debuggen twee keer zo moeilijk is als het schrijven van een programma om te beginnen. Dus als je bij het schrijven zo slim bent als je maar kunt zijn, hoe ga je het dan ooit debuggen?— Brian W. Kernighan
Elke dwaas kan code schrijven die een computer kan begrijpen. Goede programmeurs schrijven code die mensen kunnen begrijpen.— Martin Fowler, Refactoring: Improving the Design of Existing Code
Codeer altijd alsof degene die uiteindelijk je code onderhoudt een gewelddadige psychopaat is die weet waar je woont.— John Woods
Telkens wanneer ik moet nadenken om te begrijpen wat de code doet, vraag ik mezelf af of ik de code kan refactoren om dat begrip onmiddellijker duidelijk te maken.— Martin Fowler, Refactoring: Improving the Design of Existing Code
Code is slecht. Het rot. Het vereist periodiek onderhoud. Het heeft bugs die moeten worden gevonden. Nieuwe functies betekenen dat oude code moet worden aangepast. Hoe meer code je hebt, hoe meer plekken er zijn waar bugs zich kunnen verstoppen. Hoe langer checkouts of compilaties duren. Hoe langer het een nieuwe medewerker kost om je systeem te doorgronden. Als je moet refactoren is er meer spul om te verplaatsen.
Code wordt geproduceerd door ingenieurs. Om meer code te maken zijn meer ingenieurs nodig. Ingenieurs hebben n^2 communicatiekosten, en al die code die ze aan het systeem toevoegen verhoogt, terwijl het de mogelijkheden ervan uitbreidt, ook een hele reeks kosten. Je zou er alles aan moeten doen om de productiviteit van individuele programmeurs te verhogen in termen van de expressieve kracht van de code die ze schrijven. Minder code om hetzelfde te doen (en mogelijk beter). Minder programmeurs om aan te nemen. Minder organisatorische communicatiekosten.
Goede programmeurs schrijven goede code. Grootse programmeurs schrijven geen code. Zen-programmeurs verwijderen code.— John Byrd
Specifieke C-codeerlogica
Ansel en darktable zijn beide geschreven in C. Deze taal is bedoeld voor gevorderde programmeurs om snelle bugs te schrijven in besturingssystemen en applicaties op systeemniveau. Hij geeft te veel vrijheid om schadelijke dingen te doen en kan niet worden gedebugd voordat het programma wordt uitgevoerd, of voordat je je eigen tests schrijft (die zelf gebugd kunnen zijn, of het soort bugs dat ze doorlaten kunnen vertekenen, en hoe dan ook, niemand schrijft tests). Toch zijn de meeste bijdragers niet opgeleid voor C, velen van hen zijn niet eens professionele programmeurs (hoewel professionele C-programmeurs waarschijnlijk niet in de buurt van eindgebruikersapplicaties zouden moeten worden gelaten), dus C is een gevaarlijke taal voor elke opensource-app.
C laat je schrijven in buffers die niet zijn toegewezen (met een segfault-fout als gevolg) en laat je ze meer dan eens vrijgeven, maar zal buffers niet vrijgeven wanneer ze niet meer nodig zijn (met geheugenlekken als gevolg als je vergeet het handmatig te doen). Het probleem is dat, aangezien de toewijzing/vrijgave van een buffer ver weg kan liggen (in de levensduur van het programma zowel als in de broncode) van waar je ze gebruikt, het gemakkelijk is om dat te verknoeien. C laat je ook elke pointer casten naar elk gegevenstype, wat veel programmeursfouten en gegevenscorruptie mogelijk maakt. De ingebouwde methoden voor stringafhandeling zijn niet veilig (om redenen die ik nooit de moeite heb genomen te begrijpen), dus moeten we die van GLib gebruiken om beveiligingsexploits te voorkomen.
In feite maakt C je tot je eigen en ergste vijand, en het is aan jou om veiligheidsregels in acht te nemen waarvan de wijsheid pas duidelijk wordt zodra je ze overtreedt. Net als de bugs in een C-programma. Ga ervan uit dat je je code schrijft om gelezen te worden door sukkels die nog nooit eerder in C hebben geprogrammeerd.
Je moet ook in gedachten houden dat de compiler de meeste optimalisaties voor je zal doen, maar er zeer behoudend over zal zijn. De vuistregel is: als je code gemakkelijk door een mens te begrijpen is (eenvoudige logica), zal hij goed worden begrepen door de compiler, die de gepaste optimalisatiemaatregelen zal nemen. Andersom leiden handmatige optimalisaties in de code, die cryptische code opleveren waarvan wordt aangenomen dat die sneller is op single-threaded systemen, meestal tot averechtse effecten en tot tragere programma’s na compilatie.
Patronen en structuren
for-lussen zijn gereserveerd voor het itereren over arrays waarvan de grootte vooraf bekend is, zodat het aantal lusstappen bekend is. Deze logica oprekkend kunnen ze ook worden gebruikt om te itereren over GList *-items (die geen grootte-eigenschap hebben omdat ze dynamisch worden toegewezen), hoewel dit controleert of elk item (GList *)->next niet NULL is. for-lussen mogen over het algemeen geen break- of return-statements binnen hun besturingsstroom gebruiken, tenzij de lus zoekt naar een specifiek item in de array en dat retourneert zodra het is gevonden. Als je lus een stopvoorwaarde heeft, gebruik dan while.
C is geen objectgeoriënteerde taal, maar je kunt en moet OO-logica gebruiken wanneer dat relevant is in C door structuren te gebruiken om gegevens en pointers naar methoden op te slaan, en vervolgens uniforme getters en setters om de gegevens te definiëren en te benaderen.
structuren zoals while, for, if of switch mogen niet over meer dan 3 (en bij voorkeur 2) niveaus worden genest. Gebruik functies als dat gebeurt:
Lange reeksen controles moeten in een functie worden geplaatst die een gboolean retourneert en die duidelijk vermeldt wat we controleren, zodat we in procedures krijgen:
Benader gegevens uit buffers altijd met de array-achtige syntaxis, vanaf hun basispointer, in plaats van niet-constante pointers te gebruiken waarop je rekenkunde uitvoert. Doe bijvoorbeeld:
1float*const buffer =malloc(64*sizeof(float));
2for(int i =0; i <64; i++)
3{
4 buffer[i] = ...
5}
Doe niet:
1float*buffer =malloc(64*sizeof(float));
2for(int i =0; i <64; i++)
3{
4*buffer++= ...
5}
De laatste versie is niet alleen minder duidelijk te lezen, maar zal parallellisatie en compileroptimalisaties verhinderen omdat de waarde van de pointer afhangt van de lusiteratie en tussen threads gedeeld zou moeten worden als die er zijn. De eerste versie leidt tot een geheugentoegangslogica die onafhankelijk is van de lusiteratie en kan veilig worden geparallelliseerd.
Het gebruik van inline variabele-incrementen (zie hier een nachtmerrievoorbeeld ) is strikt verboden, tenzij het de enige bewerking van de regel is. Deze zijn een puinhoop die zorgt voor veel programmeerfouten. Dit is toegestaan:
1uint32_t counter;
2for(int i =0; i <64; i++)
3{
4if(array[i] > threshold)
5 counter++;
6}
De case-statements in de switch-structuur mogen niet additief zijn. Doe niet:
Bij vluchtig lezen zal het niet meteen duidelijk zijn dat het geval VALUE3 de clausules erft die door de vorige gevallen zijn gedefinieerd, vooral in situaties waar er meer gevallen zijn. Doe:
Elk geval is op zichzelf staand en de uitkomst hangt niet af van de volgorde van declaratie van de gevallen.
Sorteer en sla je variabelen op in structuren die je als functieargumenten doorgeeft in plaats van een functie met meer dan 8 argumenten te gebruiken. Doe niet:
Het eerste voorbeeld is overgenomen uit darktable . Het kopiëren en plakken van de functieaanroepen is onnodig en de vermenigvuldiging van positionele argumenten maakt het onmogelijk te onthouden welke welke is. Het toont ook niet welke argumenten constant zijn over de verschillende vertakkingen, wat refactoren moeilijk zal maken. Het laatste voorbeeld is niet beknopter, maar de structuur maakt niet alleen de functie makkelijker aan te roepen, maar de structuurdeclaratie staat toe om elk argument expliciet in te stellen, met inline controles indien nodig. De afhankelijkheid van de invoerargumenten van de externe voorwaarden wordt ook meteen duidelijk gemaakt, en de booleaanse argumenten worden rechtstreeks vanuit de voorwaarden ingesteld, wat het programma in de toekomst makkelijker uitbreidbaar en minder vatbaar voor programmeerfouten zal maken door misverstanden in de afhankelijkheid van de variabelen.
OpenMP-optimalisaties
Pixels zijn in wezen 4D RGBA-vectoren. Sinds 2004 hebben processoren speciale mogelijkheden om vectoren te verwerken en Single Instructions op Multiple Data (SIMD) toe te passen. Dit stelt ons in staat de berekeningen te versnellen door een hele pixel (SSE2) tot 4 pixels (AVX-512) tegelijk te verwerken, wat veel CPU-cycli bespaart.
Moderne compilers hebben autovectorisatie-opties die pure C kunnen optimaliseren, en de OpenMP-bibliotheek maakt het mogelijk hints te geven om dat te verbeteren, mits de code op een vectoriseerbare manier is geschreven en er enkele pragma’s worden gebruikt.
vermijd vertakkingen in lussen die de besturingsstroom veranderen. Gebruik inline statements zoals absolute = (x > 0) ? x : -x; zodat ze in SIMD kunnen worden omgezet naar byte-maskers,
pixels mogen alleen worden gerefereerd vanaf de basispointer van hun array en de indices van de lussen, zodat je kunt voorspellen welk geheugenadres wordt benaderd alleen op basis van de lusindex,
vermijd het meedragen van struct-argumenten in functies die in OpenMP-lussen worden aangeroepen, en pak de struct-leden uit vóór de lus. Vectorisatie kan niet worden uitgevoerd op structuren, maar alleen op float- en int-scalairen en -arrays. Bijvoorbeeld:
1typedef struct iop_data_t
2{
3float[4] pixel;
4float factor;
5} iop_data_t;
6 7float foo(float x, struct iop_data_t *bar)
8{
9return bar->factor * (x + bar->pixel[0] + bar->pixel[1] + bar->pixel[2] + bar->pixel[3]);
10}
1112void loop(constfloat*in, float*out, const size_t width, const size_t height, const struct iop_data_t bar)
13{
14for(size_t k =0; k < height * width; ++k)
15 {
16 out[k] = foo(in[k], bar);
17// the non-vectorized function will be called at each iteration (expensive)
18 }
19}
moet worden geschreven:
1typedef struct iop_data_t
2{
3float[4] pixel DT_ALIGNED_PIXEL; // align on 16-bits addresses
4float factor;
5} iop_data_t;
6 7#ifdef _OPENMP 8#pragma declare simd 9#endif10/* declare the function vectorizable and inline it to avoid calls from within the loop */11inline float foo(constfloat x, constfloat pixel[4], constfloat factor)
12{
13float sum = x;
1415/* use a SIMD reduction to vectorize the sum */16#ifdef _OPENMP17#pragma omp simd aligned(pixel:16) reduction(+:sum)18#endif19for(size_t k =0; k <4; ++k)
20 sum += pixel[k];
2122return factor * sum;
23}
2425void loop(constfloat*const restrict in,
26float*const restrict out,
27const size_t width, const size_t height,
28const struct iop_data_t bar)
29{
30/* unpack the struct members */31constfloat*const restrict pixel = bar->pixel;
32constfloat factor = bar-> factor;
3334#ifdef _OPENMP35#pragma omp parallel for simd default(none) \36 dt_omp_firstprivate(in, out, pixel, factor, width, height) \
37 schedule(simd:static) aligned(in, out:64)
38#endif39for(size_t k =0; k < height * width; ++k)
40 {
41 out[k] = foo(in[k], pixel, factor);
42 }
43}
als je geneste lussen gebruikt (bv. een lus over de breedte en hoogte van de array), declareer dan de pixelpointers in de binnenste lus en gebruik collapse(2) in het OpenMP-pragma zodat de compiler het cache-/geheugengebruik kan optimaliseren en de lus gelijkmatiger over de verschillende threads kan verdelen,
gebruik waar mogelijk platte indexering van arrays (for(size_t k = 0 ; k < ch * width * height ; k += ch)) in plaats van geneste breedte-/hoogte-/kanaallussen,
gebruik het restrict-sleutelwoord op afbeeldings-/pixelpointers om aliasing te vermijden en vermijd in-place bewerkingen op pixels (*out moet altijd verschillend zijn van *in) zodat je geen variabele-afhankelijkheden tussen threads veroorzaakt,
lijn arrays uit op blokken van 64 bytes en pixels op blokken van 16 bytes zodat het geheugen aaneengesloten is en de CPU volledige cacheregels kan laden (en segfaults kan vermijden),
schrijf kleine functies en optimaliseer lokaal (één lus/functie), met gebruik van OpenMP- en/of compilerpragma’s,
houd je code oerdom eenvoudig, systematisch en vermijd eigenwijze pointerrekenkunde, want dat leidt er alleen maar toe dat de compiler variabele-afhankelijkheden en pointer-aliasing detecteert waar die er niet zijn,
vermijd typecasts in de lus,
declareer invoer-/uitvoerpointers als *const en variabelen als const om false sharing in parallelle lussen te vermijden (met gebruik van het shared(variable) OpenMP-pragma).
Codeopmaak
Gebruik spaties in plaats van tabs,
Inspringing gebruikt 2 spaties,
Verwijder overtollige spaties aan het einde,
{ en } gaan op hun eigen regel,
Richtlijnen
Doe dingen die je beheerst: ja, het is leuk om nieuwe dingen te leren, maar Ansel is geen zandbak, het is productiesoftware, en het is niet de juiste plek om je training te krijgen.
KISS en wees lui: Ansel heeft geen 50 fulltime devs paraat; minimalistisch zijn zowel in functies als in codevolume is redelijk en verstandig voor het huidige beheer, maar ook voor toekomstig onderhoud. (KISS: keep it stupid simple).
Doe zoals de rest van de wereld: zeker, als iedereen uit het raam springt, heb je het recht om ze niet te volgen, maar de meeste problemen rond software-UI/UX zijn al ergens opgelost en in de meeste gevallen is het zinvol om die oplossingen simpelweg te hergebruiken, omdat de meeste gebruikers er al bekend mee zullen zijn.
Programmeren is niet het doel: programmeren is een middel tot een doel, het doel is om grote volumes foto’s in korte tijd te kunnen verwerken en tegelijkertijd op elke foto de gewenste look te bereiken. Programmeertaken moeten worden beschouwd als overhead en moeten minimaal worden gehouden, en het codevolume is een last voor elk project.
Translated from English by :
Claude.
In case of conflict, inconsistency or error, the English version shall prevail.