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

Iterate over requested_constants in DSL helpers #1989

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

KaanOzkan
Copy link
Contributor

@KaanOzkan KaanOzkan commented Aug 14, 2024

Motivation

Speedup gather_constants performance when a constant is supplied by not iterating over the ObjectSpace. This PR shouldn't change the functionality because we intersect the constants received from the ObjectSpace iteration with the requested ones

constants &= requested_constants

Running tapioca dsl Constant before and after in the monolith:
hyperfine tapioca dsl Shop (main)

hyperfine tapioca dsl Shop (perf branch)

Implementation

Tests

Most of the tests in the dsl_spec supply arguments to tapioca dsl command.

@@ -25,6 +25,8 @@ class Compiler
sig { returns(T::Hash[String, T.untyped]) }
attr_reader :options

@@requested_constants = T.let([], T::Array[Module])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to suggestions on avoiding the class variable. We want this to be set on the subclasses of Compiler which end up calling the helper methods, descendants_of or all_classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sketches me out a little, but there's a really really nice benefit to this approach: all the existing DSL Compilers will mostly just work, without needing to be updated.

What if we loosened that constraint: could we make a better API if we were willing to change the compilers?

e.g. what if the main module selection was done by something like:

sig { abstract.params(candidates: T::Enumerable[Module]).returns(T::Enumerable[Module]) }
def select_constants_to_decorate(candidates)

This would replace gather_constants. By default, we would call it with select_constants_to_decorate(all_modules) (which would work similar to how gather_constants works in practice today), otherwise we could do select_constants_to_decorate(requested_constants).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I personally don't think it's worth it just for avoiding class variables because it's a public API. I'd be happy to change default compilers inside tapioca but external ones worry me. We'd have to deprecate old usages and make everyone change their compilers eventually. Or we could have logic for both but the performance benefit would be disabled for old API which also adds maintenance complexity for us.

@KaanOzkan KaanOzkan force-pushed the ko/requested-constants-perf branch 5 times, most recently from 16c67de to c50d494 Compare August 15, 2024 17:36
@KaanOzkan KaanOzkan marked this pull request as ready for review August 15, 2024 18:09
@KaanOzkan KaanOzkan requested a review from a team as a code owner August 15, 2024 18:09
Copy link
Contributor

@amomchilov amomchilov left a comment

Choose a reason for hiding this comment

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

Here's my $0.02!

lib/tapioca/dsl/compiler.rb Show resolved Hide resolved
@@ -25,6 +25,8 @@ class Compiler
sig { returns(T::Hash[String, T.untyped]) }
attr_reader :options

@@requested_constants = T.let([], T::Array[Module])
Copy link
Contributor

Choose a reason for hiding this comment

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

This sketches me out a little, but there's a really really nice benefit to this approach: all the existing DSL Compilers will mostly just work, without needing to be updated.

What if we loosened that constraint: could we make a better API if we were willing to change the compilers?

e.g. what if the main module selection was done by something like:

sig { abstract.params(candidates: T::Enumerable[Module]).returns(T::Enumerable[Module]) }
def select_constants_to_decorate(candidates)

This would replace gather_constants. By default, we would call it with select_constants_to_decorate(all_modules) (which would work similar to how gather_constants works in practice today), otherwise we could do select_constants_to_decorate(requested_constants).

@amomchilov
Copy link
Contributor

/shipit

@KaanOzkan KaanOzkan added the refactor Code refactor with no behaviour change label Aug 27, 2024
@@ -25,6 +25,8 @@ class Compiler
sig { returns(T::Hash[String, T.untyped]) }
attr_reader :options

@@requested_constants = T.let([], T::Array[Module]) # rubocop:disable Style/ClassVars
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussion: #1989 (comment)

@KaanOzkan KaanOzkan added the enhancement New feature or request label Aug 27, 2024
@KaanOzkan KaanOzkan merged commit b6db17d into main Aug 30, 2024
30 of 31 checks passed
@KaanOzkan KaanOzkan deleted the ko/requested-constants-perf branch August 30, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Code refactor with no behaviour change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants