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

Prevent re-adding same transformers to the view all the time #381

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

Pritilender
Copy link
Contributor

@Pritilender Pritilender commented Jan 19, 2024

While updating to the latest version, I've noticed that performance for rendering a blueprint with a transformer has worsened significantly. Digging around the code, I found that the issue is the usage of concat method which keeps adding and adding same transformers to the view on each call fetching defined transformers. This change is using simple array deconstructing to ensure immutability of the transformers array.

I've also added a case with a transformer to the basic benchmark spec because I think it's valuable to also have such a case there. Let me know if I need to make it more detailed or to also add it to other benchmark specs.

Checklist:

  • I have updated the necessary documentation
  • I have signed off all my commits as required by DCO
  • My build is green

Copy link
Contributor

@lessthanjacob lessthanjacob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening a PR to address this! The change seems reasonable to me, but in the meantime, could you add some before-and-after benchmarks to the PR?

@lessthanjacob lessthanjacob added the bug An issue with the system label Jan 19, 2024
@Pritilender
Copy link
Contributor Author

The benchmark with current code from master fails after a few minutes of running:

➜ bundle exec rake benchmarks
-- create_table(:users, {:force=>:cascade})
   -> 0.0046s
-- create_table(:vehicles, {:force=>:cascade})
   -> 0.0002s
Run options: --seed 37132

# Running:


ActiveRecord IPS: 28183
.bench_render_active_record      0.000065        0.000093        0.000519        0.005195        0.050334
.
Basic IPS: 3
F

Failure:
Blueprinter::IPSTest#test_render [spec/benchmarks/ips_test.rb:32]:
Expected 3 to be >= 2500.


bin/rails test spec/benchmarks/ips_test.rb:29

bench_render_basic       0.000095        0.000128        0.000595        0.004502        0.043274
.

Finished in 163.240825s, 0.0245 runs/s, 0.0245 assertions/s.
4 runs, 4 assertions, 1 failures, 0 errors, 0 skips

After applying the changes, the benchmark passes again with the usual number for IPS on the basic benchmark:

➜ bundle exec rake benchmarks
-- create_table(:users, {:force=>:cascade})
   -> 0.0079s
-- create_table(:vehicles, {:force=>:cascade})
   -> 0.0002s
Run options: --seed 17693

# Running:

bench_render_active_record       0.000101        0.000106        0.000556        0.005278        0.053700
.
ActiveRecord IPS: 26212
.bench_render_basic      0.000049        0.000070        0.000376        0.003469        0.036867
.
Basic IPS: 27383
.

Finished in 4.610934s, 0.8675 runs/s, 0.8675 assertions/s.
4 runs, 4 assertions, 0 failures, 0 errors, 0 skips

@lessthanjacob lessthanjacob merged commit 4419e1d into procore-oss:main Jan 19, 2024
6 checks passed
@lessthanjacob lessthanjacob mentioned this pull request Jan 19, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved bug An issue with the system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants