Upgrading Discourse to Zeitwerk

개발
1

Rails 6 ships with two autoloading modes: zeitwerk and classic. In that pull request DEV: Upgrading Discourse to Rails 6 by KrisKotlarek · Pull Request #8083 · discourse/discourse · GitHub I upgraded Rails to version 6.0.0 with classic autoloader as a transitional phase. It would be interesting to try to switch to Zeitwerk.

Zeitwerk is an efficient and thread-safe code loader for Ruby. As long as the project is following naming conventions, Zeitwerk can find correct files and load them on demand or upfront without the need for any require or require_dependency. Also, it may give a small performance boost to the application according to that article https://weblog.rubyonrails.org/2019/2/22/zeitwerk-integration-in-rails-6-beta-2/

There are a few steps I need to do to make it work

  1. Change the name of few classes to follow Rails naming convention. For example file, canonical_url.rb should define CanonicalUrl class instead of CanonicalURL. Similarly file ondiff.rb should define Onpdiff class instead of ONPDiff. An alternative approach would be to hook custom inflector to the project, however, I think that following convention might be a better choice - GitHub - fxn/zeitwerk: Efficient and thread-safe code loader for Ruby · GitHub

  2. Similar to the previous point, by convention custom validations living in validations directory should be wrapped with Validations module. Besides, some validations are inheriting from EachValidator and should be accessible without namespace. I am planning to move them to separate directory and add to autoload paths.

  3. Remove all require_dependency and ensure that the project is working

  4. Remove all require and ensure that Discourse is working

  5. Ensure that all plugins can access the required dependencies. I don’t know how to achieve it yet. I want to firstly make Discourse running without any plugins.

Of course, there are still plenty of unknowns to solve. I will keep you updated about progress however if you are keen to look, I started playing with that here Commits · KrisKotlarek/discourse · GitHub

Please let me know if you see any drawbacks of implementing Zeitwerk or you think I missed something.

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

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.

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

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?

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.

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.

8

이 PR을 머지하는 작업을 시작하고 싶습니다. @kris.kotlarek 이제 머지 가능한 상태인가요? 방금 베타를 배포했으니 타이밍이 꽤 좋습니다.

10

최근 master를 해당 브랜치에 리베이스해서 여전히 정상적으로 동작하는지 확인하겠습니다. 오늘 중에 처리하겠습니다.

11

@sam 이제 진행해도 될 것 같습니다. 최신 master로 리베이스를 했고, Webauthn에 대한 작은 조정도 했습니다.
다음 3가지를 확인했습니다:
로컬에서 서버를 실행하고 조금씩 클릭하며 예상대로 작동하는지 확인
스펙 실행
모든 공식 플러그인을 다운로드하고 플러그인 스펙이 통과하는지 확인 (먼저 플러그인 조정을 병합해야 합니다)

12

좋아요, 병합하겠습니다. 어떤 결과가 나올지 지켜보죠!

13

플러그인 수정 사항도 함께 병합할 수 있을까요? 그렇지 않으면 플러그인을 설치한 상태에서 Discourse를 실행하면 실패합니다.

14

네, 병합 진행하세요!

16

이제 병합되었습니다! 플러그인 개발자 중 어려움을 겪고 계신 분이 있다면, 새로운 전용 주제에서 알려주세요!

훌륭한 작업이었어요, 크리스!!!