search

zestedesavoir / zds-site

Cœur du projet technique de Zeste de Savoir
https://zestedesavoir.com
Other
268 stars 161 forks source link

[beta 15.5.1] l'item article dans la speedbar est toujours souligné #2752

Closed firm1 closed 9 years ago

firm1 commented 9 years ago

Il n y a rien a faire, que je sois sur la home, sur la page de tous les tutoriel, sur la page des forums, etc. L'item "Articles" dans la speedbar est toujours souligné (comme on peut le voir ci-dessous).

poire

Je suppose que c'est une regression vu que ça marche très bien en prod.

Notons que en mode déconnecté, le fameux soulignement n'existe pas, et donc aussi une regresion.

GerardPaligot commented 9 years ago

Donne des informations sur ton environnement parce que je n'ai pas le bug sur OS X avec Chrome, ni FF.

firm1 commented 9 years ago

En effet.

Navigateur : Firefox 37 OS : Windows 7

SpaceFox commented 9 years ago

Zoom à 100% ? Le 26 mai 2015 11:38, "firm1" notifications@github.com a écrit :

En effet.

Navigateur : Firefox 37 OS : Windows 7

— Reply to this email directly or view it on GitHub https://github.com/zestedesavoir/zds-site/issues/2752#issuecomment-105466747 .

firm1 commented 9 years ago

Zoom a 100%, 90%, 120% => le bug reste le même.

Situphen commented 9 years ago

Problème de cache https://github.com/zestedesavoir/zds-site/blob/release-v15.5.1/templates/base.html#L175

SpaceFox commented 9 years ago

Ouais, donc en fait le front est tellement dynamique de partout qu'on peut tout de suite abandonner l'idée du cache...

pierre-24 commented 9 years ago

J'ai du mal avec cet affirmation du "front trop dynamique". Objectivement, c'est ce qu'on lui demande, c'est d'être dynamique, justement.

Alors il faut faire un choix ou repenser le front, mais si on continue comme ça, on va dans le mur.

SpaceFox commented 9 years ago

C'est une question de performances aussi. Si le moindre octet du front dépend de 12000 paramètres, il va falloir de plus gros serveurs, puisque tous ces calculs consomment et qu'on ne peut rien mettre en cache.

Pour donner une idée des temps de réponse actuels :

mois

jour

pierre-24 commented 9 years ago

cet avis n'engage que moi:

Je vois bien, et je te suis absolument, mais donc ça veut dire qu'avant de mettre en place le moindre cache, il faut repenser une partie du front. Hors, et c'est très malheureux, on manque de dev front, et j'ai l'impression qu'on tente rustine sur rustine (par exemple ça) pour quand même faire un truc potable qui se casse pas trop la gueule.

Donc pour moi, d'un coté, on est coincé par le fait qu'on doive sortir une release incessamment sous peu parce que la liste des commits s'allonge de jour en jour, d'autre part, il faudrait presque qu'on fasse un gel et qu'on prenne la peine de repenser un peu tout ça, parce qu'en l'état, on a un milliards de problèmes qui apparaissent due aux événements récents (et pas que liés au cache, je suis bien d'accord, même si j'ai l'impression que ça y est pour beaucoup, et encore une fois, aucune attaque là derrière, il FAUT qu'on mette en place un cache).

Si on agit pas incessamment sous peu (et désolé, en temps que DTC, il va falloir que tu statue là dessus), on va créer de telles incohérences que le bouzin va être impossible à maintenir.

Voilà ce que j'en pense.

SpaceFox commented 9 years ago

Ha non mais les trucs comme #2754, il ne faut surtout pas faire !

Quant au reste, le statut est déjà ici

pierre-24 commented 9 years ago

Ah, mais je parle pas simplement de la release en cours (j'avais bien vu qu'elle étais annulée et ça me semble une bonne chose à faire), je parle carrément de dire "Stop, on arrête maintenant, on fait un MP/un Zest'meeting/un post sur le forum et on réfléchis convenablement à comment intégrer le cache au front, et plus personne ne tente rien d'ici là".

Pour moi, on est déjà mal embarqué (on a pété l'API, non de non !), donc si on fait pas quelque chose, on est mal.

Peut être que je suis un peu trop alarmiste, ceci étant dit.

SpaceFox commented 9 years ago

on a pété l'API, non de non !

Même pas. L'API n'a jamais fonctionné correctement de ce point de vue, si j'en crois ce que dit @GerardPaligot ici.

Sinon, cf #2756

GerardPaligot commented 9 years ago

Même pas. L'API n'a jamais fonctionné correctement de ce point de vue, si j'en crois ce que dit @GerardPaligot ici.

Non, l'API a fonctionné. C'est à cause d'évènements récents qui m'échappent que ça ne fonctionne plus.

pierre-24 commented 9 years ago

Sinon, cf #2756

Fort bien. Mais je pense que tu n'acceptera jamais une release sans cache, donc c'est temporaire.

SpaceFox commented 9 years ago

C'est pas le cache l'important, c'est les perfs. Si on a un meilleur moyen d'avoir des perfs correctes, on a pas besoin de cache (et ce serait l'idéal).

pierre-24 commented 9 years ago

Je voudrait pas être défaitiste, mais coté back, faire mieux que ça va être très difficile. Sauf si quelqu'un est un magicien en SQL et trouve la requête qui permet de récupérer le nombre de commentaires et le dernier message avec (pour moi le truc le plus consommateur de requêtes actuellement quand je regarde la django toolbar).

pierre-24 commented 9 years ago

(après, il y a un fix facile, c'est de prefetch_*() l'image, aussi)

artragis commented 9 years ago

En terme de nombre de requête, j'ai travaillé pas mal sur ça dans la zep12 : nos pages de listes sont horribles à ce propos et on a de quoi gagner plus de 50% de perfs là !

Déjà on peut appliquer un fix très simple pour toutes les pages où il y a des messages en cassant les requêtes sur les badges de cette manière :

(fichier zds.utils.templatetags.profile.py)

perms = {'forum.change_post': {}}

@register.filter('state')
def state(user):
    try:
        profile = user.profile
        if not profile.user.is_active:
            state = 'DOWN'
        elif not profile.can_read_now():
            state = 'BAN'
        elif not profile.can_write_now():
            state = 'LS'
        elif user.pk in perms['forum.change_post'] and perms['forum.change_post'][user.pk]:
            state = 'STAFF'
        elif user.pk not in perms['forum.change_post']:
            perms['forum.change_post'][user.pk] = user.has_perm('forum.change_post')
            state = None
            if perms['forum.change_post'][user.pk]:
                state = 'STAFF'
        else:
            state = None
    except Profile.DoesNotExist:
        state = None
    return state

Ensuite, il y a des astuces à utilises dans les pages de tuto et articles et forums du style ce qui ce fait ici

queryset_reactions = ContentReaction.objects\
            .select_related('author')\
            .select_related('author__profile')\
            .select_related('editor')\
            .prefetch_related('author__post_liked')\
            .prefetch_related('author__post_disliked')\
            .prefetch_related('alerts')\
            .prefetch_related('alerts__author')\
            .filter(related_content=self.object)\
            .order_by("pubdate")

        # pagination:
        make_pagination(context,
                        self.request,
                        queryset_reactions,
                        settings.ZDS_APP['content']['notes_per_page'],
                        context_list_name='reactions',
                        with_previous_item=True)

        # is JS activated ?
        context["is_js"] = True
        if not self.object.js_support:
            context["is_js"] = False

        # optimize requests:
        reaction_ids = [reaction.pk for reaction in queryset_reactions]
        context["user_dislike"] = CommentDislike.objects\
            .select_related('note')\
            .filter(user__pk=self.request.user.pk, comments__pk__in=reaction_ids)\
            .values_list('pk', flat=True)
        context["user_like"] = CommentLike.objects\
            .select_related('note')\
            .filter(user__pk=self.request.user.pk, comments__pk__in=reaction_ids)\
            .values_list('pk', flat=True)

        if self.request.user.has_perm('forum.change_post'):
            context["user_can_modify"] = reaction_ids
        else:
            queryset_reactions_user = ContentReaction.objects\
                .filter(author__pk=self.request.user.pk, related_content__pk=self.object.pk)\
                .values_list('id', flat=True)
            context["user_can_modify"] = queryset_reactions_user

        context['isantispam'] = self.object.antispam()

pour un ordre d'idée, ces mesures ont fait passer la page publique d'un tuto de 145->185 requêtes à 45->55.

SpaceFox commented 9 years ago

La consommation du site, ce n'est pas que le nombre de requêtes, loin de là. Et je ne parle pas que de la home ; les forums sont bien pires. Générer une page à partir d'un template, c'est lourd. Souvent bien plus que les requêtes par exemple (cas de tous les articles et tous les tutos).

Et quand bien même, une page d'accueil qui a besoin de 57 requêtes en mode optimisé pour s'afficher, quelque part c'est quand même probablement qu'il y a un problème dans un coin.

Les grandes lignes sont plutôt de changer globalement sa manière de faire les choses :

SpaceFox commented 9 years ago

J'ajoute que le moteur de templates de Django est connu pour être lent. D'ailleurs Django 1.8 propose Jinja2 à la place.

artragis commented 9 years ago

Spacefox, je suis tout à fait d'accord, le nombre de requête n'est qu'une stat. C'est d'ailleurs pour ça que mon travail s'est avant tout consacré à faire du précalcul avant de faire de la réduction de requête. La réduction du nombre de requête est un effet de bord très intéressant car ce sont les requêtes répétitives qui ont été supprimées. Quand je fais 5 fois dans une page la même requête avec les mêmes paramètres c'est pas joli à voir.

Je veux bien qu'on mette du cache car il y a clairement des choses qui doivent être cachées. Mais aujourd'hui on a tellement factorisé nos templates que d'une part on ne sait pas toujours où ils se trouvent, mais en plus ça nous force à faire des tonnes de trucs paramétrisés.

Quand je vois un template comme celui qui affiche les messages d'un forum message.part.html qui a 12 niveaux d'indentation dont 7 pour des structures de contrôle (if/for/blocktrans) je me dis qu'on a un peu trop fumé quand on les a créé.

SpaceFox commented 9 years ago

Concernant les précalculs, je suis d'accord : c'est typiquement le genre de chose qu'il faut faire avant de mettre du cache.

aujourd'hui on a tellement factorisé nos templates que d'une part on ne sait pas toujours où ils se trouvent, mais en plus ça nous force à faire des tonnes de trucs paramétrisés.

Ça, c'est typique d'une refactorisation mal maîtrisée (pour rester poli). Quel que soit le langage et le contexte, si tu dois passer plein de paramètres à ta méthode générique pour gérer les cas particuliers, c'est que tu t'es planté et qu'elle n'avait pas à être générique, en tous cas pas sous cette forme.

artragis commented 9 years ago

Ça, c'est typique d'une erreur de refactorisation mal maîtrisée (pour rester poli). Quelque soit le langage et le contexte, si tu dois passer plein de paramètres à ta méthode générique pour gérer les cas particuliers, c'est que tu t'es planté et qu'elle n'avait pas à être générique, en tous cas pas sous cette forme.

on est d'accord. Perso, ce n'est pas moi qui ai factorisé le front. M'étant concentré pendant très très longtemps sur le bugfix du back, je ne saurai même pas dire qui a fait quoi sur le front, par contre je sais que les templates m'ont donné pas mal de suées sur la zep12, et c'est pas @pierre-24 qui dira le contraire.

Je veux bien mettre en pause la zep 12 pendant deux semaines pour faire quelques passages "optimisation" dans le code du dev puisque j'ai déjà travaillé sur le sujet pour la zep12 justement. Par contre je n'ai aucune expérience pour le cache. Donc faudra voir.

pierre-24 commented 9 years ago

Justement, fait sans cache, et après on verra.

Concernant les templates, c'est l'horreur intégrale. C'est pour ça que j'avais voulu attendre que l'intégration de la bêta soit terminée pour les faire sur la ZEP-12, parce que c'est du bricolage complet. Donc si quelqu'un s'amuse à faire un peu d'opti, je suis preneur :)

pierre-24 commented 9 years ago

Bon, ça a un peu dégénéré, mais le problème initial est "résolu".

Eskimon commented 9 years ago

Bon, ça a un peu dégénéré

C'est a dire ?

pierre-24 commented 9 years ago

Ben on c'est mis à discuter de cache et de solutions, quoi ;)