Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance regression after rendering refactor #361

Closed
p8 opened this issue Jan 17, 2024 · 1 comment · Fixed by #363
Closed

Performance regression after rendering refactor #361

p8 opened this issue Jan 17, 2024 · 1 comment · Fixed by #363

Comments

@p8
Copy link
Member

p8 commented Jan 17, 2024

Generating the docs is multiple factors slower on main.
I've bisected it to the following commit: d08ca4d758f2633b9a2a49a045d56d4856c2e5e4.
#312

@p8 p8 changed the title Performance regressiojn after d08ca4d758f263 Performance regression after rendering refactor Jan 17, 2024
jonathanhefner added a commit to jonathanhefner/sdoc that referenced this issue Jan 17, 2024
In d08ca4d, an isolated scope was
created for each rendered file.  That inadvertently reset memoized
values from `git` calls for each file, causing a performance regression.

This commit fixes the regression by memoizing the values globally.

With d08ca4d~1 (444c4a4):

  ```console
  $ time bundle exec rake test:rails
  real  0m31.567s
  user  0m31.159s
  sys 0m0.394s
  ```

With d08ca4d:

  ```console
  $ time bundle exec rake test:rails
  real  1m11.195s
  user  1m2.671s
  sys 0m8.811s
  ```
With d08ca4d + this commit (retrofitted):

  ```console
  $ time bundle exec rake test:rails
  real  0m26.155s
  user  0m25.796s
  sys 0m0.331s
  ```

With `main`:

  ```console
  $ time bundle exec rake test:rails
  real  1m16.000s
  user  1m7.478s
  sys 0m8.818s
```

With `main` + this commit:

  ```console
  $ time bundle exec rake test:rails
  real  0m29.964s
  user  0m29.562s
  sys 0m0.399s
  ```

Fixes rails#361.
@jonathanhefner
Copy link
Member

Thank you for finding and bisecting this! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants