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

[PROF-10112] Also simplify actionview templates with three underscores #3774

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 10, 2024

What does this PR do?

This PR is a small fix on top of #3759. In that PR, we added code to detect method names dynamically generated by Rails actionview templates, slicing the parts of the method name that have random ids.

E.g. _app_views_layouts_explore_html_haml__2304485752546535910_211320 became _app_views_layouts_explore_html_haml.

When looking at one of our example applications, I realized that I was seeing methods that ended with both __number_number as well as ___number_number (three vs two underscores), e.g.:

  • _app_views_articles_index_html_erb___2022809201779434309_12900
  • _app_views_articles_index_html_erb__3816154855874711207_12920

On closer inspection of the actionview template naming code in https://github.com/rails/rails/blob/4fa56814f18fd3da49c83931fa773caa727d8096/actionview/lib/action_view/template.rb#L389:

    def method_name # :nodoc:
      @method_name ||= begin
        m = +"_#{identifier_method_name}__#{@identifier.hash}_#{__id__}"
        m.tr!("-", "_")
        m
      end
    end

... I realized that when @identifier.hash was a negative number, it would be printed including the - sign, which would be replaced by the tr into another _. (It's somewhat weird that they didn't just go with hash.abs but yeah... >_>).

Thus, there was a 50-50 chance that methods end up with three underscores, which would make them not merge together in the flamegraph.

This PR fixes this.

Motivation:

Make sure that these dynamically-generated methods merge together properly in a flamegraph.

Additional Notes:

Here's the issue in practice -- notice the purple frame ends with both _erb and _erb_:

image

...and here's how it looked without the previous patch as well:

image

How to test the change?

This change includes test coverage.

**What does this PR do?**

This PR is a small fix on top of #3759. In that PR, we added code to
detect method names dynamically generated by Rails actionview templates,
slicing the parts of the method name that have random ids.

E.g. `_app_views_layouts_explore_html_haml__2304485752546535910_211320`
became `_app_views_layouts_explore_html_haml`.

When looking at one of our example applications, I realized that I
was seeing methods that ended with both `__number_number` as well as
`___number_number` (three vs two underscores), e.g.:

* `_app_views_articles_index_html_erb___2022809201779434309_12900`
* `_app_views_articles_index_html_erb__3816154855874711207_12920`

On closer inspection of the actionview template naming code in
https://github.com/rails/rails/blob/4fa56814f18fd3da49c83931fa773caa727d8096/actionview/lib/action_view/template.rb#L389:

```ruby
    def method_name # :nodoc:
      @method_name ||= begin
        m = +"_#{identifier_method_name}__#{@identifier.hash}_#{__id__}"
        m.tr!("-", "_")
        m
      end
    end
```

... I realized that when `@identifier.hash` was a negative number,
it would be printed including the - sign, which would be replaced by
the `tr` into another `_`. (It's somewhat weird that they didn't just go
with `hash.abs` but yeah... >_>).

Thus, there was a 50-50 chance that methods end up with three
underscores, which would make them not merge together in the flamegraph.

This PR fixes this.

**Motivation:**

Make sure that these dynamically-generated methods merge together
properly in a flamegraph.

**Additional Notes:**

N/A

**How to test the change?**

This change includes test coverage.
@ivoanjo ivoanjo requested a review from a team as a code owner July 10, 2024 08:34
@github-actions github-actions bot added the profiling Involves Datadog profiling label Jul 10, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.91%. Comparing base (d9912ed) to head (52f6cbd).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3774   +/-   ##
=======================================
  Coverage   97.91%   97.91%           
=======================================
  Files        1243     1243           
  Lines       74829    74833    +4     
  Branches     3610     3610           
=======================================
+ Hits        73272    73276    +4     
  Misses       1557     1557           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ivoanjo ivoanjo merged commit a152340 into master Jul 10, 2024
170 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/prof-10112-tweak-for-negative-hashes branch July 10, 2024 09:51
@github-actions github-actions bot added this to the 2.2.0 milestone Jul 10, 2024
@TonyCTHsu TonyCTHsu mentioned this pull request Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants