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

Improve Rails' Shape friendliness (second pass) #47032

Merged
merged 2 commits into from Jan 17, 2023

Conversation

byroot
Copy link
Member

@byroot byroot commented Jan 17, 2023

Followup: #47023

Shape Edges Report
-----------------------------------
snip...
       238  @errors
snip...
       219  @options
snip...
       129  @_request
       128  @type
       125  @virtual_path
       124  @_assigns
       123  @_config
       123  @_controller
       123  @output_buffer
       123  @view_flow
       122  @_default_form_builder
snip...
        89  @_already_called
        75  @validation_context
snip...
        65  @_new_record_before_last_commit
snip...
        58  @_url_options
snip...

@@ -205,7 +205,8 @@ def changed?(other) # :nodoc:
delegate :formats, :formats=, :locale, :locale=, :view_paths, :view_paths=, to: :lookup_context

def assign(new_assigns) # :nodoc:
@_assigns = new_assigns.each { |key, value| instance_variable_set("@#{key}", value) }
@_assigns = new_assigns
new_assigns.each { |key, value| instance_variable_set("@#{key}", value) }
Copy link
Member Author

Choose a reason for hiding this comment

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

I think ActionView::Base will inevitably be marked has having too many shapes, but at least we can keep a common root...

cc @tenderlove what do you think?

@casperisfine casperisfine force-pushed the shapes-friendliness-2 branch 3 times, most recently from f805f77 to 038835d Compare January 17, 2023 12:45
Followup: rails#47023

```
Shape Edges Report
-----------------------------------
snip...
       238  @errors
snip...
       219  @options
snip...
       129  @_request
       128  @type
       125  @virtual_path
       124  @_assigns
       123  @_config
       123  @_controller
       123  @output_buffer
       123  @view_flow
       122  @_default_form_builder
snip...
        89  @_already_called
        75  @validation_context
snip...
        65  @_new_record_before_last_commit
snip...
        58  @_url_options
snip...
```
Based on a production heap dump, I noticed that while they
didn't appear in the most common shape edges, reflections classes
were marked as having 8 or more variations.

This means they are marked as "too_complex" by MRI and fallback to
not using shapes for instance variable access.
@byroot
Copy link
Member Author

byroot commented Jan 17, 2023

ActionText test suite is broken, but that's unrelated.

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

Successfully merging this pull request may close these issues.

None yet

1 participant