Amélioration des performances

[ggrossetie]

L'interface d'écriture est plus réactive lorsqu'on modifie un article beaucoup de titres et/ou beaucoup d'éléments de bibliographies

• Utilisation de throttle pour limiter la fréquence à laquelle les statistiques et la table des matières sont régénérées (à partir du texte de l'article)
• Simplification du code
  • Utilisation de dispatch pour propager des changements d'états au store Redux
  • Améliore la lisibilité des changements d'états dans le store Redux

Ainsi que deux corrections de bug :

• la table des matières n'est plus tronquée dans la prévisualisation
• la bibliographie n'est plus réécrite par Stylo — ça cassait l'export de certains articles

[netlify]

✔️ Deploy Preview for stylo-dev ready!

🔨 Explore the source changes: 75a05a7

🔍 Inspect the deploy log: https://app.netlify.com/sites/stylo-dev/deploys/60bfced82e3b13000841122b

😎 Browse the preview: https://deploy-preview-400--stylo-dev.netlify.app

[ggrossetie]

@oncletom quelque chose semble ne pas fonctionner car le volet de gauche est maintenant (visiblement) plus lent à charger quand il y a beaucoup d'éléments dans la bibliographie !

J'utilise l'id Zotero 925666 afin de charger une bibliographie de plusieurs centaines d’éléments.

[ggrossetie]

D'après ce que je comprends quand on utilise useMemo, React appelle deux fois la fonction.

const bibTeXEntries = useMemo(() => bib2key(bib), [bib])

Il faut plutôt utiliser useEffet et useState pour que la fonction soit appelée seulement une fois :

const [bibTeXEntries, setBibTeXEntries] = useState([])

useEffect(() => {
  setBibTeXEntries(bib2key(bib))
}, [bib])

Il faudrait tester avec des composants React "simples" pour valider ce comportement.
À noter que la fonction est quand même appelée à chaque fois qu'on ouvre le volet gauche... 😞

[thom4parisot]

const bibTeXEntries = useMemo(() => bib2key(bib), [bib])

La mémoisation est "perdue" quand bib change. Y'a un truc qui fait que la valeur de bib change ?

[ggrossetie]

C'est le même article (et le même bib) mais le composant est unmount et React appelle systématiquement bib2key quand le composant est mount.

Par contre, il est possible d'utiliser le state Redux pour enregistrer l'article courant (ou du moins le BibTeX courant).
Cela permet de faire la transformation "text to entries" une seule fois:

function updateArticleBib(state, { bib }) {
  const articleBibTeXEntries = bib2key(bib)
  return { ...state, articleBib: bib, articleBibTeXEntries }
}

C'est aussi plutôt cool car cela ne bloque pas l'affichage. Si on imagine que la bibliographie n'est pas visible à l'ouverture de l'article, on peut faire la transformation en "arrière plan".

EDIT: Cela dit le problème de fond c'est qu'afficher plusieurs centaines de références bibliographiques (ici 1325) n'est pas très judicieux.
Il faudrait plutôt mettre en place une recherche/filtre afin d'éviter d'avoir à chercher dans plusieurs centaines d'éléments à afficher.

Là on "gagne" 2 secondes mais créer 1325 "composants références bibliographiques" prend aussi quelques secondes.
Dans mon exemple, j'affiche uniquement les 100 premiers éléments.

[thom4parisot]

Je serais bien partant de passer par le store Redux — dériver des données quand d'autres données changent — ça dégage beaucoup de "métier" de l'interface (affichage). Ce que tu décris comme bénéfices ne fait que renforcer ce petit effort à mettre pour rendre l'interface plus maintenable.

Ça te parle ?

[ggrossetie]

Je serais bien partant de passer par le store Redux — dériver des données quand d'autres données changent — ça dégage beaucoup de "métier" de l'interface (affichage). Ce que tu décris comme bénéfices ne fait que renforcer ce petit effort à mettre pour rendre l'interface plus maintenable.
Ça te parle ?

Totalement en phase, il y a beaucoup trop de choses dans les props et c'est déjà inmaintenable dès qu'on veut déplacer ou modifier un peu la granularité des composants car on ne sait pas ce qui se cache dans "props".

Les requêtes GraphQL polluent aussi pas mal les composants, je pense qu'extraire cette logique dans des "services" permettrait de tester plus facilement (par exemple avec des mocks).

Par exemple dans le composant Write, on peut déplacer une petite centaine de lignes pour charger l'article et remplacer par :

const data = await getArticle(articleId, applicationConfig)

Bref, je vais revert les changements qui impactent négativement les performances (useMemo) mais conserver le refactoring sur le store afin de pouvoir l’enrichir.

Je pense le faire en deux étapes pour revenir plus facilement en arrière :

1. mise au propre du store Redux
2. amélioration des performances

[ggrossetie]

@lakonis @antoinentl est-ce que vous pouvez s'il vous plait tester sur https://deploy-preview-400--stylo-dev.netlify.app qu'il n'y ait pas de regression ? Si c'est plus simple pour vous on peut aussi déployer sur https://stylo-dev.huma-num.fr/.

Lire mon premier commentaire pour la liste des changements :
#400 (comment)

C'est important pour nous de fusionner cette branche afin de débloquer les autres améliorations sur l'éditeur : édition collaborative, performance, potentiellement la refonte/simplification des onglets...

Merci !

[lakonis]

Je n'ai pas vu de problème particulier de mon côté.

Bravo à tous les deux pour les développements et pour votre workflow. C'est très rassurant (et impressionant) de vous suivre.

[ggrossetie]

@lakonis merci pour tes tests.

Bravo à tous les deux pour les développements et pour votre workflow. C'est très rassurant (et impressionant) de vous suivre.

😊

@antoinentl Est-ce que tu as eu le temps de tester ? Est-ce que tu souhaites que je déploie ces changements sur https://stylo-dev.huma-num.fr/ ?

[antoinentl]

@Mogztter Je teste depuis hier (j'ai retrouvé une connexion internet décente qui n'ajoute pas un biais) et je fais passer à l'équipe aujourd'hui !

Une question : est-ce que réduire la latence à moins d'une seconde concernant le rafraîchissement des stats casse l'objectif initial ?

[ggrossetie]

Une question : est-ce que réduire la latence à moins d'une seconde concernant le rafraîchissement des stats casse l'objectif initial ?

Il me semble que le calcul des statistiques impactent relativement peu les performances. On a amélioré les expressions régulières avec Thomas et maintenant les performances sont correctes. Cela dit c'est toujours mieux d'éviter de faire des calculs sur chaque caractère.

On peut conserver le throttle en passant la durée à 500ms ?

Je crois que les soucis de performances sont surtout liés au composant React CodeMirror2 qui est assez lent quand il y a plusieurs milliers de lignes : #404.

Il y a aussi la validation de la bibliographie qui était lancée à chaque fois même quand elle n'avait pas changé.

[ggrossetie]

@antoinentl j'ai réduis le délai à 250ms

[ggrossetie]

@antoinentl Je me permets de te ping pour que tu puisses tester ces changements. Suite à notre discussion, je propose de mettre en production le 23 juin à 09:00 UTC+2 soit 03:00 à Montréal (UTC-4). Je serais disponible jusqu'à 18:00 UTC+2 soit 12:00 à Montréal.

[antoinentl]

Ok @Mogztter , on finit de tester ces jours-ci, on sera dans les temps pour la mise en production.

[antoinentl]

Après plusieurs tests je n'ai pas vu de problème particulier ou spécifique à PR, donc c'est ok pour moi. Je re-testerai au moment de la mise en production @Mogztter 👍

[antoinentl]

@Mogztter Une mise en production à 23h30 😱

[ggrossetie]

@antoinentl désolé pour la frayeur mais le fait de fusionner une pull request sur la branche principale ne fait pas de déploiement en production. Quand un nouveau commit arrive sur la branche principale on lance un déploiement en développement sur https://stylo-dev.huma-num.fr/.

Seule les releases provoquent un déploiement en production sur https://stylo.huma-num.fr/

[antoinentl]

@Mogztter Ah oui suis-je bête, merci du rappel !

[ggrossetie]

Pas de soucis, c'est maintenant déployé en production !