ترقية Discourse إلى Zeitwerk

مطور
1

يأتي Rails 6 مع وضعين للتحميل التلقائي: zeitwerk و classic. في طلب السحب هذا https://github.com/discourse/discourse/pull/8083، قمت بترقية Rails إلى الإصدار 6.0.0 باستخدام محمل الكود الكلاسيكي كمرحلة انتقالية. سيكون من المثير للاهتمام محاولة التبديل إلى Zeitwerk.

يعتبر Zeitwerk محمل كود فعال وآمن للمواضيع (thread-safe) لـ Ruby. طالما أن المشروع يتبع اتفاقيات التسمية، يمكن لـ Zeitwerk العثور على الملفات الصحيحة وتحميلها عند الطلب أو مسبقًا دون الحاجة إلى أي require أو require_dependency. علاوة على ذلك، قد يمنح ذلك دفعة أداء صغيرة للتطبيق وفقًا لهذا المقال https://weblog.rubyonrails.org/2019/2/22/zeitwerk-integration-in-rails-6-beta-2/

هناك عدة خطوات أحتاج إلى القيام بها لجعلها تعمل:

  1. تغيير اسم بعض الفئات لتتوافق مع اتفاقية تسمية Rails. على سبيل المثال، يجب أن يحدد الملف canonical_url.rb فئة CanonicalUrl بدلاً من CanonicalURL. وبالمثل، يجب أن يحدد الملف ondiff.rb فئة Onpdiff بدلاً من ONPDiff. ستكون هناك نهج بديل وهو ربط مُعرِّف مخصص (custom inflector) بالمشروع، إلا أنني أعتقد أن اتباع الاتفاقية قد يكون خيارًا أفضل - https://github.com/fxn/zeitwerk#custom-inflector

  2. وبالمثل، وفقًا للاتفاقية، يجب تغليف عمليات التحقق المخصصة (custom validations) الموجودة في مجلد validations بوحدة Validations. بالإضافة إلى ذلك، ترث بعض عمليات التحقق من EachValidator ويجب أن تكون قابلة للوصول بدون مساحة أسماء (namespace). أخطط لنقلها إلى مجلد منفصل وإضافتها إلى مسارات التحميل التلقائي.

  3. إزالة جميع require_dependency والتأكد من عمل المشروع.

  4. إزالة جميع require والتأكد من عمل Discourse.

  5. التأكد من أن جميع الإضافات (plugins) يمكنها الوصول إلى التبعيات المطلوبة. لا أعرف كيفية تحقيق ذلك بعد. أريد أولاً جعل Discourse يعمل دون أي إضافات.

بالطبع، لا تزال هناك العديد من المجهولات التي يجب حلها. سأبقيكم على اطلاع بالتقدم المحرز، ولكن إذا كنتم ترغبون في الاطلاع، فقد بدأت في تجربة ذلك هنا https://github.com/lis2/discourse/commits/zeitwerk

يرجى إخباري إذا لاحظتم أي عيوب في تنفيذ Zeitwerk أو إذا اعتقدتم أنني أغفلت شيئًا ما.

14 إعجابًا
2

I made some progress around Zeitwerk, however, I changed my approach. My original plan was to change every place where Discourse is not following the Zeitwerk filename convention. After a few fixes, I realised that this is just the tip of the iceberg and noticed that if I am going to follow that path, then pull request will be difficult to read and confidently merge to master. For example, all job classes under regular directory should have Regular namespace, same with Onceoff and Scheduled.

I decided to step back a little bit and think about a more evolutionary approach than revolutionary.

I decided that it would be better to introduce custom Inflector, which will cover all files which are not following Zeitwerk convention. The biggest benefit will be that we would be able to deploy that small change and once we are happy with Zeitwerk and we don’t have any performance downgrades, we can start fixing convention file by file in reasonable small pull requests.

I found some problems which could not be sorted by custom Inflector, so I made additional fixes to make it work.

Pull request is still in progress, however, at this stage, I can run Discourse with Zeitwerk and default plugins, run all specs and run benchmark without a problem.

I wanted first to be at that stable state when all specs are passing. Now I can confidently start removing all require_dependency one by one and also test official plugins. Once everything is ready, I will share with you benchmark results in this post.

For now, if you are interested in progress you may take a look on that draft PR - DEV: Upgrading Discourse to Zeitwerk by KrisKotlarek · Pull Request #8098 · discourse/discourse · GitHub

The most important file is that custom Zeitwerk Inflector - DEV: Upgrading Discourse to Zeitwerk by KrisKotlarek · Pull Request #8098 · discourse/discourse · GitHub

10 إعجابات
3

To make plugins work I needed to create a few more small pull requests. Once they are merged I think that specs on Discourse should pass.

I also checked performance on Rails 6.0.0 with Classic autoloader and Rails 6.0.0 with Zeitwerk.

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

Results are not always consistent so I would take them with a grain of salt.

5 إعجابات
4

The amount of inconsistency on the median here is a bit odd, I wonder why results are fluctuating so much

Surprised loader would have any impact

4 إعجابات
5

I tried it once again, here are results

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

We can try another way to benchmark. What would you say about more iterations, something which would run for an hour? In addition, instead of taking the best result, compare the average from each experiment. That may give more consistent numbers. What do you think?

إعجاب واحد (1)
6

In my pull requests for plugins, you will notice that a lot of fixes are around searching in the global namespace.

I changed code like

module ::Jobs
  class TranslatorMigrateToAzurePortal < Jobs::Onceoff

to

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

One thing bothered me about that solution, why was it working before Zeitwerk. That question when something works but shouldn’t is always tricky :slight_smile:

I think I found a potential answer in the description of the classic autoloader (https://guides.rubyonrails.org/autoloading_and_reloading_constants_classic_mode.html#resolution-algorithms) - “If not found, then the algorithm walks up the ancestor chain of the cref”

Zeitwerk is more strict. Once I tried to load code before fix it was complaining that Jobs::Jobs::Onceoff can not be found.

Sam suggested under pull request FIX: Use top-level namespace for base classes · discourse/discourse-prometheus-alert-receiver@ef9c238 · GitHub, that instead of using < ::Jobs::Onceoff we can just use < Onceoff and he is right. I checked that without namespace it works as well. I am thinking that giving :: is explicitly saying that we are inheriting from Discourse Core class, however, we can go either way.

4 إعجابات
7

I think that when the code is this close together, this reads great:

module ::Jobs
  class TranslatorMigrateToAzurePortal < Onceoff

If the code starts floating down being explicit makes more sense … eg:

module ::Jobs
  [ 50 lines omitted]
  class TranslatorMigrateToAzurePortal < ::Jobs::Onceoff

That said, I am on a fence here, so I am fine either way. ::Jobs::Onceoff is short enough and mega explicit so we can just go with that for now.

4 إعجابات
8

I would love to start getting this merged. @kris.kotlarek is this now in a merge-able state? Timing is quite good cause we just had a beta

5 إعجابات
10

Let me rebase recent master into that and ensure that it still works, I will do that today

6 إعجابات
11

@sam I think we are good to go, I rebased with the latest master and made a small adjustment to Webauthn.
I checked 3 things:
run the server on local and click a little bit to ensure it works as expected
run specs
downloaded all official plugins and ensured that specs for plugins are passing (we need to merge adjustments for plugins first)

5 إعجابات
12

Cool I am merging this. Let’s see what shakes out!

4 إعجابات
13

Can we merge fixes for plugins as well? Otherwise, if you try to run Discourse with plugins it will fail

https://github.com/discourse/discourse-canned-replies/pull/51
https://github.com/discourse/discourse-cakeday/pull/29
https://github.com/discourse/discourse-voting/pull/45
https://github.com/discourse/discourse-perspective-api/pull/4
https://github.com/discourse/discourse-calendar/pull/4
https://github.com/discourse/discourse-prometheus/pull/4

4 إعجابات
14

Please go ahead and merge away!

3 إعجابات
16

This is now merged! If any plugin authors are struggling here, let us know in a new dedicated topic!

Great work Kris!!!

10 إعجابات