Upgrade von Discourse auf Zeitwerk

Entwickler
1

Rails 6 wird mit zwei Autoloading-Modi ausgeliefert: zeitwerk und classic. In diesem Pull Request DEV: Upgrading Discourse to Rails 6 by KrisKotlarek · Pull Request #8083 · discourse/discourse · GitHub habe ich Rails auf Version 6.0.0 mit dem klassischen Autoloader als Übergangsphase aktualisiert. Es wäre interessant zu versuchen, auf Zeitwerk umzusteigen.

Zeitwerk ist ein effizienter und threadsicherer Code-Loader für Ruby. Solange das Projekt den Namenskonventionen folgt, kann Zeitwerk die richtigen Dateien finden und sie bei Bedarf oder im Voraus laden, ohne dass require oder require_dependency erforderlich sind. Zudem kann es laut diesem Artikel https://weblog.rubyonrails.org/2019/2/22/zeitwerk-integration-in-rails-6-beta-2/ eine kleine Leistungssteigerung für die Anwendung bieten.

Es gibt einige Schritte, die ich durchführen muss, um es zum Laufen zu bringen:

  1. Den Namen einiger Klassen ändern, um der Rails-Namenskonvention zu entsprechen. Beispielsweise sollte die Datei canonical_url.rb die Klasse CanonicalUrl anstelle von CanonicalURL definieren. Ebenso sollte die Datei ondiff.rb die Klasse Onpdiff anstelle von ONPDiff definieren. Ein alternativer Ansatz wäre, einen benutzerdefinierten Inflector an das Projekt anzuhängen; ich denke jedoch, dass das Befolgen der Konvention eine bessere Wahl sein könnte – GitHub - fxn/zeitwerk: Efficient and thread-safe code loader for Ruby · GitHub

  2. Ähnlich wie im vorherigen Punkt sollten benutzerdefinierte Validierungen, die sich im Verzeichnis validations befinden, gemäß der Konvention in ein Modul Validations gekapselt werden. Außerdem erben einige Validierungen von EachValidator und sollten ohne Namensraum zugänglich sein. Ich plane, sie in ein separates Verzeichnis zu verschieben und zu den Autoload-Pfaden hinzuzufügen.

  3. Alle require_dependency-Anweisungen entfernen und sicherstellen, dass das Projekt funktioniert.

  4. Alle require-Anweisungen entfernen und sicherstellen, dass Discourse funktioniert.

  5. Sicherstellen, dass alle Plugins auf die erforderlichen Abhängigkeiten zugreifen können. Ich weiß noch nicht, wie ich dies erreichen kann. Ich möchte zunächst sicherstellen, dass Discourse ohne Plugins läuft.

Natürlich gibt es noch viele Unbekannte zu lösen. Ich werde Sie über den Fortschritt auf dem Laufenden halten. Wenn Sie jedoch Interesse haben, sich das anzusehen, habe ich hier angefangen, damit zu spielen: Commits · KrisKotlarek/discourse · GitHub

Bitte lassen Sie mich wissen, ob Sie bei der Implementierung von Zeitwerk Nachteile sehen oder ob Sie der Meinung sind, dass ich etwas übersehen habe.

2

Ich habe bei Zeitwerk Fortschritte gemacht, habe jedoch meinen Ansatz geändert. Mein ursprünglicher Plan war es, alle Stellen zu ändern, an denen Discourse nicht der Zeitwerk-Dateinamenskonvention folgt. Nach einigen Fixes wurde mir klar, dass dies nur die Spitze des Eisbergs ist, und ich stellte fest, dass ein Pull-Request auf diesem Weg schwer lesbar und kaum sicher in den Master gemergt werden könnte. Beispielsweise sollten alle Job-Klassen im regulären Verzeichnis den Regular-Namespace haben, ebenso wie Onceoff und Scheduled.

Ich habe mich entschieden, einen Schritt zurückzutreten und über einen eher evolutionären als revolutionären Ansatz nachzudenken.

Ich habe beschlossen, einen benutzerdefinierten Inflector einzuführen, der alle Dateien abdeckt, die nicht der Zeitwerk-Konvention entsprechen. Der größte Vorteil besteht darin, dass wir diese kleine Änderung bereitstellen können. Sobald wir mit Zeitwerk zufrieden sind und keine Performance-Einbußen auftreten, können wir die Konvention schrittweise, Datei für Datei, in vernünftig kleinen Pull-Requests korrigieren.

Ich habe einige Probleme gefunden, die sich durch einen benutzerdefinierten Inflector nicht lösen ließen, und habe zusätzliche Fixes vorgenommen, damit es funktioniert.

Der Pull-Request befindet sich noch in Arbeit, aber in diesem Stadium kann ich Discourse mit Zeitwerk und den Standard-Plugins ausführen, alle Specs laufen lassen und Benchmarks ohne Probleme durchführen.

Zuerst wollte ich einen stabilen Zustand erreichen, in dem alle Specs bestehen. Jetzt kann ich selbstbewusst beginnen, alle require_dependency-Aufrufe nacheinander zu entfernen und auch die offiziellen Plugins zu testen. Sobald alles bereit ist, werde ich die Benchmark-Ergebnisse in diesem Beitrag mit euch teilen.

Falls ihr am Fortschritt interessiert seid, könnt ihr euch diesen Entwurf-Pull-Request ansehen: DEV: Upgrading Discourse to Zeitwerk by KrisKotlarek · Pull Request #8098 · discourse/discourse · GitHub

Die wichtigste Datei ist der benutzerdefinierte Zeitwerk-Inflector: DEV: Upgrading Discourse to Zeitwerk by KrisKotlarek · Pull Request #8098 · discourse/discourse · GitHub

3

Damit die Plugins funktionieren, musste ich noch einige kleinere Pull Requests erstellen. Sobald diese gemergt sind, sollten meiner Meinung nach die Tests in Discourse bestehen.

Ich habe zudem die Leistung unter Rails 6.0.0 mit dem klassischen Autoloader und unter Rails 6.0.0 mit Zeitwerk überprüft.

Test Klassisch Zeitwerk Prozent
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

Die Ergebnisse sind nicht immer konsistent, daher sollte man sie mit einem Körnchen Salz nehmen.

4

Die Inkonsistenz auf dem Median ist hier etwas seltsam, ich frage mich, warum die Ergebnisse so stark schwanken.

Überraschend, dass der Loader irgendeinen Einfluss haben sollte.

5

Ich habe es erneut versucht, hier sind die Ergebnisse

Test Classic Zeitwerk Prozent
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

Wir könnten einen anderen Ansatz für den Benchmark versuchen. Was hältst du von mehr Durchläufen, sodass der Test eine Stunde läuft? Außerdem könnten wir statt des besten Ergebnisses den Durchschnitt aus jedem Experiment vergleichen. Das könnte konsistentere Zahlen liefern. Was meinst du?

6

In meinen Pull Requests für Plugins wirst du feststellen, dass viele Korrekturen die Suche im globalen Namensraum betreffen.

Ich habe Code wie

module ::Jobs
  class TranslatorMigrateToAzurePortal < Jobs::Onceoff

in

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

geändert.
Etwas hat mich an dieser Lösung gestört: Warum funktionierte es vor Zeitwerk? Diese Frage, wenn etwas funktioniert, obwohl es nicht sollte, ist immer knifflig :slight_smile:

Ich glaube, ich habe eine mögliche Antwort in der Beschreibung des klassischen Autoloaders gefunden (https://guides.rubyonrails.org/autoloading_and_reloading_constants_classic_mode.html#resolution-algorithms) – „Wird nichts gefunden, durchläuft der Algorithmus die Vorfahrenkette des crefs“.

Zeitwerk ist strenger. Sobald ich versuchte, Code zu laden, bevor ich ihn korrigierte, beschwerte es sich darüber, dass Jobs::Jobs::Onceoff nicht gefunden werden kann.

Sam schlug im Pull Request FIX: Use top-level namespace for base classes · discourse/discourse-prometheus-alert-receiver@ef9c238 · GitHub vor, dass wir statt < ::Jobs::Onceoff einfach < Onceoff verwenden können, und er hat recht. Ich habe geprüft, dass es auch ohne Namensraum funktioniert. Ich denke, das angeben von :: bedeutet explizit, dass wir von einer Discourse-Core-Klasse erben, aber wir können beides wählen.

7

Ich finde, wenn der Code so nah beieinander steht, liest sich das super:

module ::Jobs
  class TranslatorMigrateToAzurePortal < Onceoff

Wenn der Code jedoch weiter unten steht, macht es mehr Sinn, explizit zu sein … z. B.:

module ::Jobs
  [ 50 Zeilen ausgelassen]
  class TranslatorMigrateToAzurePortal < ::Jobs::Onceoff

Trotzdem bin ich hier noch unschlüssig, also ist beides für mich in Ordnung. ::Jobs::Onceoff ist kurz genug und mega explizit, also können wir vorerst einfach dabei bleiben.

8

Ich würde mich sehr freuen, wenn wir das jetzt zusammenführen könnten. @kris.kotlarek, ist das jetzt in einem zusammenführbaren Zustand? Der Zeitpunkt passt gut, da wir gerade ein Beta-Release hatten.

10

Ich werde den aktuellen Master dort neu aufsetzen und sicherstellen, dass alles noch funktioniert. Ich erledige das heute.

11

@sam Ich denke, wir sind startklar. Ich habe mit dem neuesten Master neu aufgesetzt und eine kleine Anpassung an Webauthn vorgenommen.
Ich habe drei Dinge geprüft:
Den Server lokal ausgeführt und ein wenig herumgeklickt, um sicherzustellen, dass er wie erwartet funktioniert
Spezifikationen ausgeführt
Alle offiziellen Plugins heruntergeladen und sichergestellt, dass die Spezifikationen für die Plugins durchlaufen (wir müssen zuerst die Anpassungen für die Plugins zusammenführen)

12

Cool, ich mergge das. Mal sehen, was dabei herauskommt!

13

Können wir auch Fehlerbehebungen für Plugins zusammenführen? Andernfalls schlägt der Versuch, Discourse mit Plugins auszuführen, fehl.

14

Bitte einfach mergen!

16

Das ist jetzt gemergt! Falls Plugin-Autoren hier Probleme haben, teilt uns das bitte in einem neuen, dafür vorgesehenen Thema mit!

Tolle Arbeit, Kris!!!