poniedziałek, 16 lipca 2012

Dlaczego ważne jest by nie używać ponownie tych samych zmiennych lokalnych

W tym poście chciałbym pokazać do czego prowadzi pewna "dziadowska oszczędność" czyli mikrooptymalizacja w kodzie.

W ostatnim poście, opisałem niektóre zalecane (o zgrozo) mikrooptymalizacje które mają uczynić twój kod nieczytelnym i przy okazji zwiększyć jego wydajność (autor zapomniał dopisać że będzie to rzędu 3-4%).

Właśnie przed chwilą szukałem dość poważnego błędu w dość starym kawałku mojego kodu (połowa węzłów drzewa miała wartość null, zamiast liczby nieparzystej).


    /**
     * Metoda rekurencyjnie zaglebia sie w drzewo i tworzy strukture drzewiastą o zadanym stopniu głębokosci, wywolujemy ja
     * dla kazdego węzła poddrzewa
     *
     * @param int      $level            poziom głębokosci drzewa, gdy dojdziemy do 1 to Metoda sie zatrzymuje
     * @param bool     $insertNullFields Czy Metoda na tym poziomie ma wstawic puste miejsce na nowego przegranego czy nie
     * @param TreeNode $node             galaz dla ktorej bedziemy przeprowadzac operacje
     */
    protected function createChildNode($maxLevel, $insertNullFields, TreeNode $node, $currentLevel,
                                       $numeratorTableCounter = null, $roundID = null)
    {
        if ($roundID < 10 || $roundID == null) {
            echo "błąd";
        }

        if ($currentLevel == $maxLevel) {
            /* Dla obydwu dzieci tego węzła ktore beda ostatnimi węzłami w tym drzewie */
            for ($counter = 0; $counter < 2; $counter++) {
                /* Przypisz dla obecnie edytowanego węzła nowy węzeł jako dziecko, ID weź z numeratora poniewaz są
        * to puste węzły z miejscem na przegranych odpadajacych z turnieju, algorytm sie zatrzymuje,
        * dla dzieci dalej nie robimy juz nic */
                $newNode = TreeNode::create($this->nodes, $node, null, null, null, $roundID);
                /* Przypisz ten wezeł jako pojemnik na przegranych na odpowiednim poziomie i pozycji */
                $this->numeratorTable[$numeratorTableCounter][] = $newNode->ID;
            }
        } else {
            if ($currentLevel < $maxLevel) {
                /* Wstawiamy jedno puste pole i jedno sie dalej rozgałęzia, uruchomiono Metode z atrybutem True */
                if ($insertNullFields) {
                    /* Puste pole */
                    $newNode = TreeNode::create($this->nodes, $node, null, null, null, $roundID);
                    $node->children[0] = $newNode;
                    /* Wpisujemy do tabelki z polami przegranych */
                    $this->numeratorTable[$numeratorTableCounter][] = $newNode->ID;
                    /* Pole rozgaleziajace sie dalej (miejsce na bitwe i na jej wynik) */
                    $newNode = TreeNode::create($this->nodes, $node);
                    /* I dla drugiego dziecka wywolujemy rekurencyjnie tą Metode, tym razem z atrybutem "false", poziom zmniejszamy */
                    $this->createChildNode($maxLevel, false, $newNode, ($currentLevel + 1),
                        ($numeratorTableCounter + 1), ($roundID - 1));
                } else {
                    /* Metoda uruchomiona z atrybutem false (wstaw 2 galezie i im dzieciom tez uruchom ta Metode) */
                    for ($counter = 0; $counter < 2; $counter++) {
                        $newNode = TreeNode::create($this->nodes, $node, null, null, null, $roundID);
                        /* uruchamiamy tą Metode dla nowo wstawionego dziecka, z atrybutem true (wstaw jedno pole pełne i jedno puste) */
                        $this->createChildNode($maxLevel, true, $newNode, ($currentLevel + 1),
                            ($numeratorTableCounter), ($roundID - 1));
                    }
                }
            }
        }
    }
ok, to jest kod całej metody, nie będziemy się zagłębiać w to czy jest dobrze napisana, bo wiedza zgodnie z prawem moora rośnie wykładniczo z czasem, co daje taki efekt, że człowiek nie chce patrzeć na kod który pisał rok temu bo uważa go za shit.

oto kawałek który był błędny
//[...]
 if ($currentLevel < $maxLevel) {
                /* Wstawiamy jedno puste pole i jedno sie dalej rozgałęzia, uruchomiono Metode z atrybutem True */
                if ($insertNullFields) {
                    /* Puste pole */
                    $newNode = TreeNode::create($this->nodes, $node, null, null, null, $roundID);
                    $node->children[0] = $newNode;
                    /* Wpisujemy do tabelki z polami przegranych */
                    $this->numeratorTable[$numeratorTableCounter][] = $newNode->ID;
                    /* Pole rozgaleziajace sie dalej (miejsce na bitwe i na jej wynik) */
// TUTAJ JEST BŁĄD - powinno być w parametrze jeszcze null,null, $roundID
                    $newNode = TreeNode::create($this->nodes, $node);
                    /* I dla drugiego dziecka wywolujemy rekurencyjnie tą Metode, tym razem z atrybutem "false", poziom zmniejszamy */
                    $this->createChildNode($maxLevel, false, $newNode, ($currentLevel + 1),
                        ($numeratorTableCounter + 1), ($roundID - 1));
                }
//[...]


jak widać mamy tutaj zmienną newNode użytą w dwóch miejscach.

pierwsze użycie to

$newNode = TreeNode::create($this->nodes, $node, null, null, null, $roundID);
natomiast drugie to

$newNode = TreeNode::create($this->nodes, $node); // notabene ta druga również powinna mieć jeszcze 3 parametry: null,null, $roundID

W pierwszym momencie, pomyślałem sobie, (to dośc logiczny wniosek) że, po prostu omyłkowo mi się skopiował kod z pierwszego wywołania w miejsce drugiego, tylko jeszcze do tego wcieło mi 3 parametry i obiekt dobry zostal zastąpiony złym. W końcu jest to zrobione do jednej zmiennej $newNode.

Dopiero wnikliwsza analiza pozwoliła mi stwierdzić, że tak naprawde to powinny być utworzone dwie TreeNode, jedna na góre i jedna na dół. Pierwsza jest tworzona w $treeNode, zostaje dodana do drzewa, następnie druga TreeNode zostaje przypisana do $treeNode i również dodana do drzewa. Jednak te obiekty powinny się one znajdować w dwóch zmiennych - $upNewNode i $downNewNode, wtedy po spojrzeniu na metodę od razu bym wiedział że są to dwa rozdzielne obiekty, a nie jeden ciągły.

Dlatego nie powinno się "reużywać" starych lokalnych zmiennych dla nowych obiektów - jeżeli chcemy pisać kod poprawny semantycznie i taki który da się rozszyfrować nawet jeśli nie ma komentarzy - a komentarze z kodu chcemy w miare mozliwości usuwać i zastępować je czytelnym kodem - podwójnie użyta zmienna nie pozwala na odczytanie jej sensu z samego kodu. Nawet jeśli pierwszy obiekt jest uzywany w pierwszej części metody, a drugi w drugim. Panowie i panie! Robimy dwie zmienne, nie oszczędzamy na 2 bajtach pamięci bo później są z tego tytułu same problemy.

piątek, 6 lipca 2012

Google Code Optimizations: Avoid getters and setters

Wielki szacun dla Google. Jest to świetna, i bardzo innowacyjna firma, ich produkty również zazwyczaj były bardzo dobre, i co ważne działały w różnych regionach świata, czego czasem nie można było powiedzieć o konkurencji. Od strony technicznej raczej rzadko można było się do czegoś przyczepić, google nie sypało critical errorami ani nie znikało z netu. Aby to osiągnąć, google (taki jest mój obecny stan wiedzy na ten temat) korzystało z mieszanki pythona, C, C++, i chyba jeszcze jakiegoś języka. W każdym razie, nie było tam PHP.

I oto google wypuszcza nam artykuł, jak optymalizować skrypty PHP, by wykonywały się szybciej.

Optimizing PHP scripts speed with Google

Do pierwszych kilku porad nic nie mam. Profilowanie kodu, memcache (gdy rozwiążemy problem konkurencji w dostępie do cache uruchamianych w wielu wątkach skryptów PHP), może znacząco podnieśc wydajność.

I oto trafiamy na najlepszą poradę
Avoid writing naive setters and getters When writing classes in PHP, you can save time and speed up your scripts by working with object properties directly, rather than writing naive setters and getters. In the following example, the dog class uses the setName() and getName() methods for accessing the name property.

Setting and calling the name property directly can run up to 100% faster, as well as cutting down on development time.


co za gniot. postaram się wykazać jego bezsensowność.

Załóżmy że robimy klasę. Prostą klasę, ma dwa pola, z jakichś powodów nie można ich ustawić w konstruktorze.

Podążąjąc za paradygmatem OOP w którym pole klasy jest jego wewnętrznym szczegółem implementacji, i nie powinien być publiczny by w przyszłości móc go zmienić bez łamania API klasy, powinniśmy uczynić te pola prywatnymi, i dorobić do nich akcesory get/set.

Jednak jest to wolne, ble, i w ogóle trzeba się kupe czasu opisać (swoją drogą - czy ten facet z google widział kiedyś na oczy choćby darmowe Eclipse PDT czy Netbeans PHP, który ten kod generuje sam z siebie?), więc nie piszemy akcesorów, zaoszczędzamy procesor, pamięć oraz czas.

public class Example
{
   public $fieldOne;
   public $fieldTwo;
}


Klasa idzie w świat, albo choćby rozpełza się po projekcie, jej API zostaje użyte w kilkudziesięciu miejscach w kodzie. Przypominam że język PHP to język z dynamicznym typowaniem, który jeśli nie posiadamy bardzo dobrego edytora, błędów typów nam po prostu nie wyłapie (zresztą zaawansowane narzędzia czasami również tego nie potrafią niestety zrobić).

Nagle z jakiegos powodu okazuje się że jedno pole powinno być stringiem, a drugie integerem z zakresu 1-100. Co można zrobić w takiej sytuacji?

Mamy dwa wyjścia. Bardziej wydajne (wg Google): Dopisać PHPdoc informujący o tym że jedno z pól jest stringiem, a drugie int. W komentarzu dopisujemy że wartość spoza zakresu 1-100 spowoduje błąd.

Drugie wyjście. Dorobić akcesory i zrobić w nich walidację typu (php nie ma statycznie typowanych zmiennych/pól ale można wymusić typy w metodach, więc akcesory mogą służyć niejako walidacji).

No i teraz mamy problem. I to spory. Trzeba znaleźć wszystkie użycia danej klasy, zamienić $obj->fieldOne, na $obj->getFieldOne() i tak samo z setami. Jestem niemal przekonany że nie znajdziesz wszystkich od razu. Odczytasz sobie nulla z nieistniejącego pola w klasie, i php nawet nie pomyśli żeby cię o tym powiadomić. I będziesz debugował 2 godziny logikę, zastanawiając się dlaczego tablica zwraca -1, wartość która miała być stringiem jest liczbą float w dodatku jest równa minus ileśtam, tylko dlatego że głupie odwołanie do publicznego pola zwróciło ci null zamiast wartości.

Warto było? Mam w swoich kodach pare takich klas które się rozeszły po całym projekcie i mają publiczne pola. I te pola to teraz spory problem, ale nawet nie podejmuje się zamiany ich na akcesory, nie mając kodu pokrytego testami jednostkowymi chociaż w 90%. Po prostu teraz muszę pamiętać że jakiejś zmiennej się nie ustawia na -1 bo to wysypie jakiś program, a inne pole jest tylko do odczytu, chociaż jest publiczne (bez akcesorów nie da się zrobić pól readonly w PHP).

Tak się kończy zabawa w mikrooptymalizacje. Akcesor nie jest taki zły, nawet jeśli robi to samo co publiczne pole. Nigdy nie wiadomo czy będzie taki w przyszłości. Czy w kolejnej wersji programu nie będziesz chciał dodać w tym miejscu lazy-load albo podmienić algorytm na bardziej efektywny. Upubliczniając wewnętrzne szczegóły klasy jakim są pola nie da się tego zrobić bezboleśnie. Klasa nie powinna mieć publicznych pól choćby nie wiem co! Chyba że jest tylko niezbyt inteligentnym DTO, pojemnikiem na dane które muszą mieć swoje nazwy, ale nawet tam gettery i settery mogą się przydać.. być może nie teraz a kiedyś.

Jeśli stosujesz się do konwencji że wszystkie properties klasy które możesz odczytać zaczynasz od get... a wszystko co możesz zapisać od set.. to po wpisaniu do IDE $obj->get otrzymujesz elegancką listę tego co możesz przypisać, analogicznie z set. Mając mix akcesorów i publicznych pól cięzko rozróżnić co jest propertiesami a co zwykłymi metodami.

Oczywiście gettery i settery są złe. Znacznie lepiej jest robić klasy które wykonują czynności wewnątrz siebie, pobierając po prostu parametry, niż udostępniać prywatne pola w pełni publicznymi akcesorami, i manipulować nimi na zewnątrz klasy (choć przy niektórych wzorcach projektowych się tego używa, np visitor).

class Zla
{
   private $predkosc;
   private $odleglosc;
   public function getOdleglosc(){ return $this->odleglosc; }
   public function setOdleglosc($value) { $this->odleglosc=$value; }
   public function getPredkosc() {return $this->predkosc;}
   public function setPredkosc($value) {$this->predkosc=$value;}
}

$zla = new Zla();
$zla->setPredkosc(10);
$zla->setOdleglosc(20);
// itd itp


class Dobra
{
    private $predkosc;
    private $odleglosc;
    public function getOdleglosc(){ return $this->odleglosc;}
    public function getPredkosc() {return $this->predkosc;}

    public function przejedzKawalek($odleglosc, $predkosc)
    {
       // logika jazdy
    }
}

$dobra=new Dobra();
$dobra->przejedzKawalek(10,20);
// itd itp


Jednak należy zwrócić uwagę że tutaj motywacją do usunięcia setterów jest semantyka, a nie pseudooptymalizacje wydajnościowe i lenistwo programisty któremu nie chce sie wcisnąć CTRL+G.

Dodatkowym argumentem za stosowaniem getterow i setterów jest to że IDE łatwiej zrobi rename metod akcesorów, jeśli zmieni się nieco ich znaczenie, niż znajdzie wszystkie miejsca gdzie użyłeś danego pola i zamieni to na mutator.

Zresztą, dbanie o wydajność w trakcie pisania kodu, w takim języku jakim jest PHP (jest to jeden z najwolniejszych języków programowania), zakrawa na ironię. Napisz, tak żeby było czytelnie i bezbłędnie, odpal profiler, popraw jedną metodę która zajmuje 90% czasu skryptu tak żeby było szybko, ciesz się wydajnością. Profilowanie aplikacji ma sens. Mikrooptymalizacje nie. Getter poprawia nieco i tak kulejącą w php kontrolę nad poprawnością kodu. On ci chociaż wywali błąd, przynajmniej dowiesz sie o nim, nie będziesz musiał zgadywac co jest nie tak.
Profilowaniem często można osiągnąć poprawe wydajności znacznie lepszą (i nie zaburzającą czytelności) niż psuciem i zasmiecaniem kodu korzystając ze średniowiecznych technik kodowania. Zresztą, jak chcesz żeby było szybko, to pisz w assemblerze. Po co ci PHP? Assembler zrobi to najszybciej.

W przypadku aplikacji webowych i tak najczęściej wąskim gardłem jest baza danych, więc te pseudooptymalizacje dadzą wymiernie mały efekt przy dużym wysiłku.

Pewnie mało z programistów PHP miało styczność z językiem C#. W tym języku akcesory można deklarować bardzo łatwo
class One
{
    // to jest odpowiednik prostego getPole setPole w javie/PHP
    public int Pole1 {get;set;}
    // w razie potrzeby np. walidacji lub lazy loadu, lub reagowania na zmiane jakiegoś pola
    // możemy rozwinąć powyższy property do postaci poniższej, bez zmieniania interfejsu klasy
    private int _pole2;
    public int Pole2 
    {
       get { return this.doSomethingWithPole2OnGet(); }
       set { this.setSomethingInPole2(value); }
    }
    private int doSomethingWithPole2OnGet()
    {
        // zrob z polem co chcesz zanim zwrocisz. Nie trzeba robic oddzielnej metody
        // ale jest to w C# dobra praktyka by jakies bardziej skomplikowane
        // operacje nie byly w akcesorach tylko w metodach uzywanych w nich.
    }
    private void setSometingInPole2(int value)
    {
        // j.w.
    }
}
Nigdy nie widziałem dyskusji przeciwko getterom i setterom w C#. Ludzie po prostu z nich korzystają. Niby mały lukierek składniowy, mamy do wpisania(wygenerowania) kilka znaków {get;set;} a nie całą litanię getPole/setPole, a ludzie już inaczej zupełnie podchodzą do tego. A może po prostu ludzie piszący w C# mając do dyspozycji szybki język kompilowany, nie muszą poprawiać jego wydajności pseudooptymalizacjami i mogą sie skupic na czytelności, a może po prostu PHP jak lep przyciąga złe praktyki programistyczne? Być może coś w tym jest. Być może się powinno spędzić troche czasu przy javie/C# aby nauczyć się dobrej obiektowości, i dopiero siąść do PHP który ma tą obiektowość momentami wybrakowaną, która sprzyja wyrabianiu złych nawyków.

Nie zajmę stanowiska bo nie chcę flamewara :)

Reasumując - jeśli masz dobry powód do usunięcia gettera/settera z kodu to je usuń, jednak nie rób tego z lenistwa lub dla wydajności. A to jest po prostu piękne.
Don't copy variables for no reason Sometimes PHP novices attempt to make their code "cleaner" by copying predefined variables to variables with shorter names before working with them. What this actually results in is doubled memory consumption (when the variable is altered), and therefore, slow scripts. In the following example, if a user had inserted 512KB worth of characters into a textarea field. This implementation would result in nearly 1MB of memory being used. $description = strip_tags($_POST['description']); echo $description; There's no reason to copy the variable above. You can simply do this operation inline and avoid the extra memory consumption: echo strip_tags($_POST['description']);
No proszę. A ja myślałem że wolne skrypty to takie które nie keszują ciężkich zapytań do bazy. Albo robią 10 razy to samo, gdy wystarczy raz.

Czy ten człowiek zastanawiał się kiedyś dlaczego coś mu w kodzie nie działa, i która tablica w tym przepięknym łańcuchu
   echo getFromDb(intval(strip_tags($_POST['description'.$id.'_'.$fieldName]))+$id);
mu zwraca -1 i dlaczego? Ale to taki piękny i czysty kod, jest idealny, wszystko inline, nie ma żadnych zbędnych zmiennych. Nie wiem jak jemu, mi jest szkoda czasu na wpisywanie po kolei wszystkiego w watch, lub rozbijanie tych wyrażeń na poszczególne zmienne gdy znajde w nich błąd. Ja wole to.
   $fieldName= 'description'.$id.'_'.$fieldName;
   $valueInPost = $_POST[$fieldName];
   $strippedValue = strip_tags($valueInPost);
   $intValueOfStripped = intval($strippedValue);
   $idOfRow = $intValueOfStripped+$id;
   $queryResult = getFromDb($idOfRow);
   echo $queryResult;


Zaraz, zaraz.. czyżbyśmy właśnie przyoszczędzili na komentarzach, które do pierwszego przykładu trzeba by było nieuchronnie napisać?
Kod skomentował się sam. I to jest świetne. Bo gdybym napisał komentarz, jestem prawie pewien ze gdybym coś pozmieniał, to komentarz zostałby niezmieniony i za miesiąc wprowadziłby mnie w błąd.
Ustawię breakpoint na queryResult, uruchomię skrypt, będę miał w IDE podgląd wartości wszystkich zmiennych, szybko znajdę tą która popsuła mi skrypt. Bo raczej we wnętrzu strip_tags nie umieszcze breakpointa...

Zresztą, Zend engine w PHP5 ma pewien mechanizm który powoduje że renundantne przypisanie
    $fieldName2 = $fieldName;
    $fieldName3= $fieldName2;
    doSmth($fieldName3);
nie zużyje pamięci na dodatkowe zmienne $fieldName2 i 3. Także spokojnie można poprawiać czytelnośc kodu dodatkowymi zmiennymi. Zresztą, nawet jakby miało zużywać pamięć to i tak wolę tą czytelną wersję.
Jak już pisałem wcześniej, jeżeli podczas profilowania okaże się że ten kawałek wykonuje sie przez 90% czasu skryptu, to go zoptymalizuję, napisze nieczytelnie ale tak żeby był szybki.
Wcześniej nie.
A tu już bardziej zmyślony przykład, ale takie czasami się zdarzają wewnątrz klas które modelują rzeczywiste procesy i muszą operować na logice
    $is_ended = $tournament->isEnded || ($tournament->state = STATE_LEAGUE_CHECK && $tournament->time > time()) || ($tournament->state = STATE_TOURNAMENT_SIGNIN && $tournament->signedTeams > 30) && ........ ;


 // a ja wolę tak
  $tournamentIsEnded = $tournament->isEnded;
  $tournamentIsInCheckin = $tournament->state = STATE_LEAGUE_CHECK && $tournament->time > time();
  $maxAmountOfTeams = 30;
  $tooMuchSignedTeamsToTournament = $tournament->state = STATE_TOURNAMENT_SIGNIN && $tournament->signedTeams > $maxAmountOfTeams;
  $is_ended= $tournamentIsEnded || $tournamentIsInCheckin || $tooMuchSignedTeamsToTournament ||... ;
Czy nie lepiej sie czyta takie coś? Czy może mam płakać że skrypt mi zajmie 20 kb więcej w pamięci, pomimo że wykona się bezbłędnie?
Jeśli 20kb pamięci robi ci różnicę, to jak już pisałem może czas dokupić nowy serwer, albo napisać ten moduł w C skoro jest taki ważny, jak to robił Facebook zanim napisał HipHopa?
Już w "Clean Code" było jak wół napisane że wyrażenia logiczne są i tak trudne, nie komplikuj ich. Czy tak trudno przenieść zalecenia napisane dla javy, na grunt PHP którego obiektowość jest w dużej mierze skopiowana z javy? Dużo ludzi wpadło na taki pomysł ale ten spec od optymalizacji już nie.

PHP5 jest mocno inspirowany Javą. Java powstała aby zapewnić przenośność kodu w stosunku do C++, oraz, by wyeliminować źródła najczęstszych błędów programistycznych jakie zdarzały się programistom C++, dlatego zawierała szereg usprawnień. PHP upodobniło swoją obiektowość do javowej, więc weźmy przykłąd z nich i piszmy tak by błędów było mniej a nie więcej.
Bo wedle praw murphyego jeśli gdzieś da się popełnić błąd, to błąd zostanie popełniony.

Jeszcze wcześniej google błysneło zajebistą poradą aby zastępować nawiasy
   $query =  "select $fields from $table where $id=$valueOfID";

na konstrukcje
   $query = 'select '.$fields.' from '.$table.' where '.$id.'='.$valueOfID';

ponieważ PHP musi parsować podwójne nawiasy i taki skrypt wykona sie 100% wolniej (czyli 0.0005ms) Najwyraźniej to że string zapisany w ten sposób jest kompletnie nieczytelny i błąd składniowy wynikający z braku spacji czy apostrofu w ważnym miejscu, oraz fakt że samo zapytanie będzie się wykonywać pewnie z 20ms, czyli 40000x dłużej jest pomijalny.
Dlatego ja mam gdzieś mikrooptymalizacje i robię to sposobem do którego "zaraził" mnie Mateusz Świetlicki, choć początkowo miałem opory w stylu "to będzie wolno działało"
   sprintf('select %s from %s where %s=%s', $fields,$table,$id,$valueOfId);

tutaj już mi się cudzysłowy czy spację nie pomylą bo mam oddzielony template stringa od jego zawartości.


Zbliżam się do podsumowania posta. A podsumowanie jest takie: wszystkie optymalizacje wg Google które tutaj naświetliłem w artykule robiłem mając bardzo krótki staż w PHP, myślałem że robię dobrze, a w miarę jak zauważałem do czego to prowadzi, zastępowałem je (lub po prostu nie pisałem tak więcej) solucjami które tu pokazałem. Być może też troche było w tym inspiracji C#. W każdym razie zdenerwowało mnie że ktoś pokazuje błędy beginnerów jako dobre praktyki.

PS. jak patrze na te kawałki kodu z artykułu google, to mam wrażenie że pisali je ci sami programiści którzy pisali moduły do joomli 1.5. Kto się bawił ten będzie wiedział jak wyglądają od środka i dlaczego są takie zabawne :)