Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1 requête par membre pour en afficher les infos (toute pages) #2089

Closed
SpaceFox opened this issue Jan 19, 2015 · 18 comments

Comments

Projects
None yet
3 participants
@SpaceFox
Copy link
Member

commented Jan 19, 2015

La page de listing des membres fait une requête de ce type par membre affiché :

SELECT `member_profile`.`id`, `member_profile`.`user_id`, `member_profile`.`last_ip_address`, `member_profile`.`site`, `member_profile`.`show_email`, `member_profile`.`avatar_url`, `member_profile`.`biography`, `member_profile`.`karma`, `member_profile`.`sign`, `member_profile`.`show_sign`, `member_profile`.`hover_or_click`, `member_profile`.`email_for_answer`, `member_profile`.`sdz_tutorial`, `member_profile`.`can_read`, `member_profile`.`end_ban_read`, `member_profile`.`can_write`, `member_profile`.`end_ban_write`, `member_profile`.`last_visit` FROM `member_profile` WHERE `member_profile`.`user_id` = <id membre>

Autant dire, la plupart du temps, 100 requêtes !

Cause : zds-site/templates/misc/member_item.part.html ligne 5 : {% with profile=member|profile %}

C'est pas grand chose, mais ça ne devrait pas être trop compliqué à corriger.

PS : ceci semble toucher tout affichage d'infos membre, cf infra.

@SpaceFox SpaceFox added S-BUG C-Back Facile and removed Facile labels Jan 19, 2015

@SpaceFox SpaceFox added this to the "Futur lointain" (2.0+) milestone Jan 19, 2015

@GerardPaligot

This comment has been minimized.

Copy link
Member

commented Jan 19, 2015

Ce bug est corrigé par la ZEP 17 (qui fera bientôt l'objet d'une PR). Je l'ai corrigé ce soir.

En fait, on envoyait la liste des User au template et, pour chaque user, on demandait le profile. Maintenant, la liste des profiles est directement envoyée.

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2015

@GerardPaligot j'ai l'impression que ça concerne tous les affichages des membres partout sur le site (et pas que cette page), tu confirmes ?

@GerardPaligot

This comment has been minimized.

Copy link
Member

commented Jan 20, 2015

La requête pour récupérer le profile de chaque utilisateur était présent dans le fichier member_item.part.html qui possède 32 occurrences dans l'ensemble des templates. Donc, c'est possible oui.

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2015

OK, donc je ne m'occupe pas de ça et attends la ZEP 17 :)

@GerardPaligot

This comment has been minimized.

Copy link
Member

commented Jan 21, 2015

J'ai un peu mesuré les répercussions de cette issue et elle est bien plus importante que je ne le pensais (qui va bien au delà de la ZEP 17).

En fait, tous les modèles n'utilisent jamais Profile mais User. Il est donc toujours nécessaire de récupérer le profile dans les templates.

Pour éviter de faire ces requêtes, il faudrait non plus avoir des relations vers User mais vers Profile à chaque fois. Les répercutions sont justes monstrueuses.

Je viens demander l'avis aux devs pour savoir quoi faire ?

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2015

Avant de continuer, pour être sûr :

  • User est le modèle par défaut de Django
  • Profile est en relation 1:1 avec User et contient nos infos spécifiques sur l'utilisateur
  • Si on récupère User puis qu'on cherche son Profile, on fait 1 requête spécifique pour récupérer les infos dans la table profile
  • Par contre, si on récupère un Profile, une jointure est systématique pour récupérer le User correspondant

J'ai bon ?

@GerardPaligot

This comment has been minimized.

Copy link
Member

commented Jan 21, 2015

Ouaip.

@GerardPaligot

This comment has been minimized.

Copy link
Member

commented Jan 21, 2015

Bien, devant l'importance de la modification (et des choix techniques à discuter), j'ai contourné le problème pour la ZEP 17 et ne résoudrait donc pas cette issue dans la foulée.

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2015

Donc il faut prévoir une PR géante pour ce problème ?

Genre, pas compliquée, mais longue et fastidieuse ?

@GerardPaligot

This comment has been minimized.

Copy link
Member

commented Jan 21, 2015

Exactement.

@SpaceFox SpaceFox changed the title Page des membres : 1 requête par membre pour en afficher les infos 1 requête par membre pour en afficher les infos (toute pages) Jan 24, 2015

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2015

Je viens de jeter un oeil et effectivement... la PR va être massive. Entre autres parce que toutes nos clés étrangères se font sur User et non Profile !

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2015

En fait, j'ai dit complètement de la merde dans mon post d'avant. L'ORM de Django est intelligent et sait parfaitement gérer les relations 1..1 entre User et Profile.

Du coup il suffit de coller des .select_related("profile") à chaque fois qu'on appelle un User pour l'affichage et ça marche sans autre forme de modification. Je prépare une PR avec ça.

@SpaceFox SpaceFox self-assigned this Jan 31, 2015

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2015

Hmmm... mon truc ne marche que dans certains cas. Y'a encore des récupérations tordues. Je vais attendre que la v1.5 soit déployée, déjà ça me dégagera les listes de requêtes de pas mal de merde.

Mais je commence à avoir une bonne idée de ce qu'il faut faire.

@gustavi

This comment has been minimized.

Copy link
Member

commented Jan 31, 2015

Alors j'ai la solution après avoir discuté avec des devs de Django. En gros il faut surcharger le modèle User. @SpaceFox si ça t'intéresse, je peux faire une POC.

Note : ça veut également dire toucher le modèle qui est en relation avec tous les autres modèle donc ça va demander une QA lourde.

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2015

Ah ouais... vu les impacts, je pense qu'il faut garder l'idée sous le coude, mais uniquement si on arrive à rien par d'autres biais. Parce qu'on sortirait complètement des recommandations officiels Django, et qu'on se retrouverait avec une migration de données particulièrement immonde.

@gustavi

This comment has been minimized.

Copy link
Member

commented Jan 31, 2015

Bah en fait non (pour on sortirait complètement des recommandations officiels Django). Surcharger le modèle semble être quelque chose de courant (j'avoue ne jamais avoir eu à le faire) mais ça se fait à la création d'un projet.

Actuellement ce qu'il faudrait faire c'est de modifier une méthode de User pour ajouter un .select_related('profile'). Ensuite on modifie les imports un peu partout et ça passe.

@SpaceFox

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2015

Je pense comprendre.

En fait, le code de PdP fonctionnait avec la méthode AUTH_PROFILE_MODULE, dépréciée lors du passage à Django 1.6.

Ce que tu proposes revient à la substitution de modèle utilisateur, que je n'avais pas appliquée à cause du gros avertissement qui dit qu'il faut commencer le projet avec.

J'ai appliqué l'extension de modèle utilisateur, sauf que du coup notre façon de l'utiliser provoque des quantités délirantes de requêtes.

On a donc deux choses à faire ici :

  1. Réduire le nombre de fois où on va chercher un profil spécifique (au sens large : User + Profile), parce que ça quel que soit le modèle ça va faire une requête dédiée au moins.
  2. S'arranger pour ne jamais récupérer des demi-profiles quand on a besoin du profile complet (récupération de User sans son Profile ou inversement).

Ça te paraît cohérent ?

@gustavi

This comment has been minimized.

Copy link
Member

commented Jan 31, 2015

Oui ça me parait tout à fait cohérent ! Ce qu'il faut c'est surtout avoir User et Profile toujours liés ! Ça ne changera pas grand chose en DB étant donné qu'on a presque toujours besoin des deux.

SpaceFox added a commit to SpaceFox/zds-site that referenced this issue Feb 5, 2015

SpaceFox added a commit to SpaceFox/zds-site that referenced this issue Feb 5, 2015

SpaceFox added a commit to SpaceFox/zds-site that referenced this issue Feb 6, 2015

SpaceFox added a commit to SpaceFox/zds-site that referenced this issue Feb 7, 2015

@SpaceFox SpaceFox closed this in 17ed01a Feb 9, 2015

gustavi added a commit that referenced this issue Feb 9, 2015

Merge pull request #2221 from SpaceFox/fix_2089
Fixes #2089: Amélioration des perfs sur l'affichage de membres

@gustavi gustavi modified the milestones: Version 1.6, "Futur lointain" (2.0+) Feb 9, 2015

Situphen added a commit to Situphen/zds-site that referenced this issue Feb 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.