Mise à niveau de Discourse vers Zeitwerk

Dev
1

Rails 6 est livré avec deux modes d’autoloading : zeitwerk et classic. Dans cette pull request DEV: Upgrading Discourse to Rails 6 by KrisKotlarek · Pull Request #8083 · discourse/discourse · GitHub, j’ai mis à niveau Rails vers la version 6.0.0 en utilisant l’autoloader classic comme phase de transition. Il serait intéressant d’essayer de passer à Zeitwerk.

Zeitwerk est un chargeur de code efficace et sûr pour les threads en Ruby. Tant que le projet respecte les conventions de nommage, Zeitwerk peut trouver les fichiers corrects et les charger à la demande ou immédiatement, sans avoir besoin d’utiliser require ou require_dependency. De plus, cela peut offrir un léger gain de performance à l’application, selon cet article : https://weblog.rubyonrails.org/2019/2/22/zeitwerk-integration-in-rails-6-beta-2/

Voici quelques étapes que je dois effectuer pour que cela fonctionne :

  1. Changer le nom de quelques classes pour respecter la convention de nommage de Rails. Par exemple, le fichier canonical_url.rb devrait définir la classe CanonicalUrl au lieu de CanonicalURL. De même, le fichier ondiff.rb devrait définir la classe Onpdiff au lieu de ONPDiff. Une approche alternative consisterait à attacher un inflector personnalisé au projet, mais je pense que respecter la convention est un meilleur choix : GitHub - fxn/zeitwerk: Efficient and thread-safe code loader for Ruby · GitHub

  2. De même que pour le point précédent, par convention, les validations personnalisées situées dans le répertoire validations doivent être enveloppées dans le module Validations. Par ailleurs, certaines validations héritent de EachValidator et doivent être accessibles sans espace de noms. Je prévois de les déplacer dans un répertoire séparé et de les ajouter aux chemins d’autoloading.

  3. Supprimer tous les require_dependency et s’assurer que le projet fonctionne.

  4. Supprimer tous les require et s’assurer que Discourse fonctionne.

  5. S’assurer que tous les plugins peuvent accéder aux dépendances requises. Je ne sais pas encore comment y parvenir. Je souhaite d’abord faire fonctionner Discourse sans aucun plugin.

Bien sûr, il reste encore de nombreuses inconnues à résoudre. Je vous tiendrai informé de l’avancement, mais si vous souhaitez jeter un coup d’œil, j’ai commencé à expérimenter cela ici : Commits · KrisKotlarek/discourse · GitHub

Merci de me faire part de tout inconvénient que vous voyez à l’implémentation de Zeitwerk ou si vous pensez que j’ai oublié quelque chose.

2

J’ai fait des progrès autour de Zeitwerk, mais j’ai changé d’approche. Mon plan initial était de modifier chaque endroit où Discourse ne respecte pas la convention de nommage des fichiers de Zeitwerk. Après quelques corrections, j’ai réalisé qu’il ne s’agissait que de la partie émergée de l’iceberg et j’ai remarqué que si je suivais cette voie, la demande de fusion (pull request) serait difficile à lire et à fusionner en toute confiance dans la branche master. Par exemple, toutes les classes de tâches situées dans un répertoire standard doivent avoir l’espace de noms Regular, de même pour Onceoff et Scheduled.

J’ai décidé de faire un pas en arrière et de réfléchir à une approche plus évolutive que révolutionnaire.

J’ai décidé qu’il serait préférable d’introduire un Inflecteur personnalisé, qui couvrira tous les fichiers ne respectant pas la convention de Zeitwerk. Le plus grand avantage sera que nous pourrons déployer ce petit changement et, une fois que nous serons satisfaits de Zeitwerk et qu’il n’y aura aucune baisse de performance, nous pourrons commencer à corriger la convention fichier par fichier dans des demandes de fusion raisonnables et de petite taille.

J’ai rencontré certains problèmes que l’Inflecteur personnalisé ne pouvait pas résoudre, j’ai donc apporté des corrections supplémentaires pour que cela fonctionne.

La demande de fusion est toujours en cours, mais à ce stade, je peux exécuter Discourse avec Zeitwerk et les plugins par défaut, lancer tous les tests (specs) et exécuter le benchmark sans problème.

Je voulais d’abord atteindre cet état stable où tous les tests réussissent. Maintenant, je peux commencer en toute confiance à supprimer tous les require_dependency un par un et tester également les plugins officiels. Une fois que tout sera prêt, je partagerai avec vous les résultats du benchmark dans ce message.

Pour l’instant, si vous êtes intéressé par les progrès, vous pouvez consulter cette ébauche de demande de fusion : DEV: Upgrading Discourse to Zeitwerk by KrisKotlarek · Pull Request #8098 · discourse/discourse · GitHub

Le fichier le plus important est cet Inflecteur Zeitwerk personnalisé : DEV: Upgrading Discourse to Zeitwerk by KrisKotlarek · Pull Request #8098 · discourse/discourse · GitHub

3

Pour que les plugins fonctionnent, j’ai dû créer quelques petites pull requests supplémentaires. Une fois qu’elles seront fusionnées, je pense que les spécifications sur Discourse devraient passer.

J’ai également vérifié les performances avec Rails 6.0.0 et le chargeur automatique Classic, ainsi qu’avec Rails 6.0.0 et Zeitwerk.

Test Classic Zeitwerk Pourcentage
categories-50 32 26 81,25
categories-75 37 29 78,38
categories-90 47 35 74,47
categories-99 67 49 73,13
home-50 30 29 96,67
home-75 37 31 83,78
home-90 44 40 90,91
home-99 67 52 77,61
topic-50 35 35 100,00
topic-75 36 36 100,00
topic-90 48 36 75,00
topic-99 57 58 101,75
categories_admin-50 51 48 94,12
categories_admin-75 62 50 80,65
categories_admin-90 89 66 74,16
categories_admin-99 135 101 74,81
home_admin-50 48 47 97,92
home_admin-75 58 49 84,48
home_admin-90 67 64 95,52
home_admin-99 101 81 80,20
topic_admin-50 48 48 100,00
topic_admin-75 55 49 89,09
topic_admin-90 63 65 103,17
topic_admin-99 92 69 75,00
load_rails 2617 2165 82,73
rss_kb 282428 315684 111,78
pss_kb 270491 303504 112,20

Les résultats ne sont pas toujours cohérents, il faut donc les prendre avec un grain de sel.

4

Le niveau d’incohérence sur la médiane ici est un peu étrange, je me demande pourquoi les résultats fluctuent autant.

Je suis surpris que le loader ait un quelconque impact.

5

Je l’ai essayé à nouveau, voici les résultats

Test Classique Zeitwerk Pourcentage
categories-50 25 25 100,00
categories-75 26 26 100,00
categories-90 37 33 89,19
categories-99 57 48 84,21
home-50 26 26 100,00
home-75 27 28 103,70
home-90 38 35 92,11
home-99 60 50 83,33
topic-50 27 26 96,30
topic-75 35 27 77,14
topic-90 41 33 80,49
topic-99 54 50 92,59
categories_admin-50 48 50 104,17
categories_admin-75 60 61 101,67
categories_admin-90 76 71 93,42
categories_admin-99 122 122 100,00
home_admin-50 47 46 97,87
home_admin-75 58 55 94,83
home_admin-90 66 63 95,45
home_admin-99 99 121 122,22
topic_admin-50 50 49 98,00
topic_admin-75 62 50 80,65
topic_admin-90 72 65 90,28
topic_admin-99 103 74 71,84
load_rails 2675 2216 82,84
rss_kb 279924 315240 112,62
pss_kb 267659 303026 113,21

Nous pouvons essayer une autre méthode pour les tests de performance. Qu’en pensez-vous : plus d’itérations, avec une durée d’exécution d’une heure ? De plus, au lieu de prendre le meilleur résultat, comparons la moyenne de chaque expérience. Cela pourrait donner des chiffres plus cohérents. Qu’en pensez-vous ?

6

Dans mes pull requests pour les plugins, vous remarquerez que de nombreuses corrections concernent la recherche dans l’espace de noms global.

J’ai modifié du code comme :

module ::Jobs
  class TranslatorMigrateToAzurePortal < Jobs::Onceoff

en

module ::Jobs
  class TranslatorMigrateToAzurePortal < ::Jobs::Onceoff

Une chose m’a interpellé avec cette solution : pourquoi cela fonctionnait-il avant Zeitwerk ? Cette question, quand quelque chose fonctionne mais ne devrait pas, est toujours délicate :slight_smile:

Je pense avoir trouvé une réponse potentielle dans la description du chargeur automatique classique (https://guides.rubyonrails.org/autoloading_and_reloading_constants_classic_mode.html#resolution-algorithms) : « Si non trouvé, l’algorithme remonte alors la chaîne des ancêtres du cref ».

Zeitwerk est plus strict. Dès que j’ai essayé de charger le code avant la correction, il se plaignait que Jobs::Jobs::Onceoff ne pouvait pas être trouvé.

Sam a suggéré dans la pull request FIX: Use top-level namespace for base classes · discourse/discourse-prometheus-alert-receiver@ef9c238 · GitHub que, au lieu d’utiliser < ::Jobs::Onceoff, nous puissions simplement utiliser < Onceoff, et il a raison. J’ai vérifié que cela fonctionne également sans espace de noms. Je pense que l’utilisation de :: indique explicitement que nous héritons d’une classe du cœur de Discourse, mais nous pouvons choisir l’une ou l’autre option.

7

Je pense que lorsque le code est aussi rapproché, cela se lit très bien :

module ::Jobs
  class TranslatorMigrateToAzurePortal < Onceoff

Si le code commence à s’étaler sur plusieurs lignes, être explicite a plus de sens… par exemple :

module ::Jobs
  [ 50 lignes omises]
  class TranslatorMigrateToAzurePortal < ::Jobs::Onceoff

Cela dit, je suis indécis à ce sujet, donc les deux options me conviennent. ::Jobs::Onceoff est suffisamment court et très explicite, nous pouvons donc opter pour cette solution pour l’instant.

8

J’aimerais beaucoup commencer à fusionner cela. @kris.kotlarek, est-ce que c’est maintenant dans un état fusionnable ? Le timing est tout à fait opportun car nous venons d’avoir une bêta.

10

Laissez-moi réappliquer la branche master récente sur celle-ci et m’assurer qu’elle fonctionne toujours. Je le ferai aujourd’hui.

11

@sam Je pense que nous sommes prêts. J’ai fait un rebase avec la dernière version de master et apporté un petit ajustement à Webauthn.
J’ai vérifié trois points :
lancer le serveur en local et tester un peu pour m’assurer qu’il fonctionne comme prévu
exécuter les spécifications
télécharger tous les plugins officiels et m’assurer que les spécifications pour les plugins passent (nous devons d’abord fusionner les ajustements pour les plugins)

12

Super, je fusionne ça. On verra bien ce que ça donne !

13

Pouvons-nous également fusionner les correctifs pour les plugins ? Sinon, si vous essayez d’exécuter Discourse avec des plugins, cela échouera.

14

Allez-y et fusionnez !

16

C’est maintenant fusionné ! Si des auteurs de plugins rencontrent des difficultés, faites-le nous savoir dans un nouveau sujet dédié !

Excellent travail, Kris !!!