Mapper with keywords #52512

gmcgibbon · 2024-08-05T22:07:06Z

Motivation / Background

Followup #52422

Detail

This Pull Request changes the routing mapper to use keywords instead of hashes in most places, which results in a slight speed boost, and clearer code.

Additional information

Using the benchmark in the initial deprecation PR:

Before:

ruby 3.3.1 (2024-04-23 revision c56cd86388) [arm64-darwin23]
Warming up --------------------------------------
                draw   113.000 i/100ms
Calculating -------------------------------------
                draw      1.110k (± 3.1%) i/s -      5.650k in   5.095601s

After:

ruby 3.3.1 (2024-04-23 revision c56cd86388) [arm64-darwin23]
Warming up --------------------------------------
                draw   116.000 i/100ms
Calculating -------------------------------------
                draw      1.174k (± 3.2%) i/s -      5.916k in   5.042676s

It turns out the real speed difference is next to nothing, which is unfortunate. There was a bug in my initial implementation that made it faster 🤦. However, I think now that the mapper is easier to reason about, we should be able to do see optimization potential a little bit easier. I can see two in the scopes and AST parsing that I'll work on next.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

tenderlove

Seems good, just a couple questions.

tenderlove · 2024-08-05T22:14:41Z

actionpack/lib/action_dispatch/routing/mapper.rb

-            options.delete(app) if app
-            hash_key_app = true
-          end
+        def mount(app, as: nil, via: nil, at: nil, defaults: nil, constraints: nil, anchor: false, format: false, path: nil, internal: nil, **mapping, &block)


what is **mapping? A method call to something?

Mapping is route mapping parameters as a keyword splat like:

rails/actionpack/lib/action_dispatch/routing.rb

Lines 133 to 137 in 9ba208c

# get '/articles/:year/:month/:day', to: 'articles#find_by_id', constraints: {

# year: /\d{4}/,

# month: /\d{1,2}/,

# day: /\d{1,2}/

# }

To be honest, I'm not familiar with this usage outside of constraints. We might want to deprecate this too.

Yeah actually I think this is just a shorthand for constraints:

rails/actionpack/test/controller/routing_test.rb

Lines 566 to 567 in 9ba208c

get "page/:year/:month/:day/:title", to: "page#show", as: "article",

year: /\d+/, month: /\d+/, day: /\d+/

These params get passed down to the route that gets built too, which they might not need to be:

rails/actionpack/lib/action_dispatch/routing/mapper.rb

Line 144 in 9ba208c

options = ast.wildcard_options.merge!(options)

I'll dig a little deeper and see what's going on with this usage. Thanks!

As far as I can tell, strings passed as mapper parameters are treated as route defaults, and regexes are constraints for route parameters. I vote to deprecate this since there's already keywords for this.

I think I can try to deal with mapping parameter deprecations above in a future PR. I need to introduce keywords in resource calls too which could be done as a followup or now.

actionpack/test/dispatch/routing_test.rb

gmcgibbon · 2024-08-05T22:32:20Z

actionpack/test/dispatch/routing_test.rb

        get "/c/:id", as: :collision, to: "collision#show"
        get "/collision", to: "collision#show"
-        get "/no_collision", to: "collision#show", as: nil
+        get "/no_collision", to: "collision#show", as: false


This might be a regression. The previous behaviour is get "/no_collision", to: "collision#show", as: nil wouldn't name the route and now it will because keyword defaults are nil.

Fixed in the latest commit. I don't like this usage either IMO so I'd like to deprecate this behaviour too IMO.

rafaelfranca · 2024-08-06T17:17:00Z

We can't merge this one right? We can't remove behavior that we just deprecated. We can only merge this after Rails 8.0 is released.

gmcgibbon · 2024-08-06T20:26:22Z

We can only merge this after Rails 8.0 is released.

Correct. I might be able to make it work without removing the deprecations so we can get the benefits sooner. It might look weird, so I'll take a look.

gmcgibbon · 2024-08-08T02:46:45Z

There, this should now be mergable today. The deprecations added can be removed in the next release to clean up the router.

Main:

ruby 3.3.1 (2024-04-23 revision c56cd86388) [arm64-darwin23]
Warming up --------------------------------------
                draw   127.000 i/100ms
Calculating -------------------------------------
                draw      1.280k (± 1.9%) i/s -      6.477k in   5.062105s

This branch:

ruby 3.3.1 (2024-04-23 revision c56cd86388) [arm64-darwin23]
Warming up --------------------------------------
                draw   129.000 i/100ms
Calculating -------------------------------------
                draw      1.316k (± 1.9%) i/s -      6.579k in   5.002806s

Same-ish difference with the deprecations re-added and the regression above patched.

Converts hashes to keywords in ActionDispatch::Routing::Mapper. This makes the mapper a little easier to reason about, and slightly faster.

jhawthorn · 2024-08-21T03:00:35Z

actionpack/lib/action_dispatch/routing/mapper.rb

-            raise ArgumentError, "Route path not specified" if path.nil?
-
+        def match(*path_or_actions, as: DEFAULT, via: nil, to: nil, controller: nil, action: nil, on: nil, defaults: nil, constraints: nil, anchor: nil, format: nil, path: nil, internal: nil, **mapping, &block)
+          if path_or_actions.count > 1


I don't think the conditional here matches what the previous code was doing and the deprecation inside doesn't seem to match having multiple actions specified (which I'm also not clear on whether that should even be deprecated).

…rds" This reverts commit a27076d, reversing changes made to e13b251.

rails-bot bot added the actionpack label Aug 5, 2024

gmcgibbon force-pushed the mapper_with_keywords branch from 9ae462c to 29f5205 Compare August 5, 2024 22:12

tenderlove reviewed Aug 5, 2024

View reviewed changes

gmcgibbon commented Aug 5, 2024

View reviewed changes

gmcgibbon mentioned this pull request Aug 7, 2024

Make route mapping scope lookups faster #52525

Merged

4 tasks

gmcgibbon force-pushed the mapper_with_keywords branch from 29f5205 to be8d9ff Compare August 8, 2024 02:45

gmcgibbon force-pushed the mapper_with_keywords branch from be8d9ff to 214022e Compare August 8, 2024 02:53

Use keywords in routing mapper

6be2367

Converts hashes to keywords in ActionDispatch::Routing::Mapper. This makes the mapper a little easier to reason about, and slightly faster.

gmcgibbon force-pushed the mapper_with_keywords branch from 518478c to 6be2367 Compare August 10, 2024 02:48

gmcgibbon merged commit a27076d into rails:main Aug 10, 2024

gmcgibbon deleted the mapper_with_keywords branch August 10, 2024 04:55

This was referenced Aug 14, 2024

Use keywords with scopes and resources #52605

Merged

Remove racc #52610

Merged

tagliala mentioned this pull request Aug 20, 2024

Rails Edge support enriclluelles/route_translator#314

Closed

jhawthorn reviewed Aug 21, 2024

View reviewed changes

jhawthorn added a commit to jhawthorn/rails that referenced this pull request Aug 21, 2024

Revert "Merge pull request rails#52512 from Shopify/mapper_with_keywo…

de1e6c9

…rds" This reverts commit a27076d, reversing changes made to e13b251.

jhawthorn mentioned this pull request Aug 21, 2024

Revert keyword argument changes to routing mapper #52664

Merged

DanielaVelasquez pushed a commit to DanielaVelasquez/rails that referenced this pull request Oct 3, 2024

Revert "Merge pull request rails#52512 from Shopify/mapper_with_keywo…

191c4b9

…rds" This reverts commit a27076d, reversing changes made to e13b251.

gmcgibbon mentioned this pull request Nov 21, 2024

Use keywords in routing mapper #53689

Merged

4 tasks

	# get '/articles/:year/:month/:day', to: 'articles#find_by_id', constraints: {
	# year: /\d{4}/,
	# month: /\d{1,2}/,
	# day: /\d{1,2}/
	# }

	get "page/:year/:month/:day/:title", to: "page#show", as: "article",
	year: /\d+/, month: /\d+/, day: /\d+/

Mapper with keywords #52512

Mapper with keywords #52512

Uh oh!

Conversation

gmcgibbon commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation / Background

Detail

Additional information

Checklist

Uh oh!

tenderlove left a comment

Choose a reason for hiding this comment

Uh oh!

tenderlove Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

gmcgibbon Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmcgibbon Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmcgibbon Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

gmcgibbon Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmcgibbon Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmcgibbon Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelfranca commented Aug 6, 2024

Uh oh!

gmcgibbon commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmcgibbon commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhawthorn Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gmcgibbon commented Aug 5, 2024 •

edited

Loading

gmcgibbon Aug 5, 2024 •

edited

Loading

gmcgibbon Aug 5, 2024 •

edited

Loading

gmcgibbon Aug 8, 2024 •

edited

Loading

gmcgibbon Aug 5, 2024 •

edited

Loading

gmcgibbon Aug 9, 2024 •

edited

Loading

gmcgibbon commented Aug 6, 2024 •

edited

Loading

gmcgibbon commented Aug 8, 2024 •

edited

Loading