patacrep / patanet

Web interface for LaTeX songbook generation
GNU Affero General Public License v3.0
10 stars 3 forks source link

Performance fix #114

Closed oliverpool closed 9 years ago

oliverpool commented 9 years ago

Cette branche n'est pas encore prête à être mergée.

J'ai fait quelques modifications qui avaient pour but principal de réduire le nombre de requête SQL: il faut à tout prix que le nombre de requêtes ne soit pas linéaire en fonction du nombre d'éléments affichés sur la page, mais bel et bien indépendant.

Sur la page qui liste les Carnets de chant, je suis passé de 58 à moins de 8 requêtes !! (de 150ms à moins de 30ms) Et encore, je n'avais que 6 carnets de chant...

Un gros inconvénient, c'est que le code est moins facilement lisible...c'est notamment cela qui me fait écrire que cette branche n'est pas encore prête à être mergée...

Luthaf commented 9 years ago

C'est cool ! Le code étant commenté, il est plutôt lisible. Ça vaudrait peut-etre le coup de passer les commentaires initiaux en docstring, mais à part ça tout me semble bien.

oliverpool commented 9 years ago

Le nom de la fonction _add_attr_numbers me parait quand même pas top... Peut-être qu'il faudrait séparer le tout en 3 fonction distincts qui renvoient inv_songs, inv_artists et inv_sections Avec une fonction de sélection, puis une fonction qui attache ces valeurs aux songbooks. Pour avoir un truc du style:

for item_type in ['songs', 'artists', 'sections']:
  counted = _quick_count(item_type, songbooks)
  _attach_attr('num_' + item_type, songbooks)

Avec _quick_count_songs, _quick_count_artists, _quick_count_sections en sous-fonctions

oliverpool commented 9 years ago

@Luthaf : du coup j'ai créé une classe Quick_Counter, mais je ne sais pas du tout où la ranger...

Luthaf commented 9 years ago

J'aurai plutôt ajouté 3 fonctions, ou bien une seule avec un paramètre item_type dans le modèle songbook. Ces fonctions sont liés à une instance particulière de ce modèle, non ?

oliverpool commented 9 years ago

item_type est pas mal, mais il n'y en a pas pour l'artiste Mais avec une chaine de caractères pour item_type et un switch case, ça peut bien se faire effectivement

oliverpool commented 9 years ago

C'est bon, j'ai éclaté la classe en 3 fonctions:

def _count_item_of_type(item_string_type, songbooks)
def _count_item_of_type_artist(songbooks)
def _count_and_attach_as_attributes(item_types, songbooks)
Luthaf commented 9 years ago

def _count_item_of_type(item_string_type, songbooks)

Je ne mettrais pas le type dans le nom. item_type est suffisant, on est en Python et vive le duck-typing !

Sinon ça me semble bien.

oliverpool commented 9 years ago

Ok, je corrige Dans mon esprit, c'était pour éviter des confusions entre "section", Section et ContentType.objects.get(app_label="generator", model="section") qui sont tous des item_type

Edit, fait : 93d2a33

oliverpool commented 9 years ago

Je n'ai pas d'autre idées d'améliorations dans l'immédiat : si ça convient on peut merger!

oliverpool commented 9 years ago

Il suffit que j'écrive

Je n'ai pas d'autre idées d'améliorations dans l'immédiat

pour que je trouve une autre idée... L'insertion et la suppression de chants est assez inefficace