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

Lazy lookup and lazy object #2002

Merged
merged 9 commits into from
Mar 1, 2020

Conversation

ericproulx
Copy link
Contributor

Hello,

I'm still working on improving string allocations and this PR follows my work.

  • I added some lazy lookup tables at some places for a better string allocations.
  • I added a lazy object which kind of a proxy for executing code when needed. It's mainly used when building headers for request. I also added a lazy object const that contains converted well known http headers.
  • I fixed a small leak in stackable_values for empty arrays. It's now generating empty_array only when needed
  • I replaced some %w by actual const

Here's some stats based on our app:

Overall Grape object allocations (development environment)

# master

Total allocated: 47115631 bytes (437131 objects)
Total retained:  6323963 bytes (16355 objects)

# this PR
Total allocated: 41291815 bytes (300978 objects)
Total retained:  5841323 bytes (11641 objects)

benchmark/simple.rb ruby 2.4.9

# master
eproulx@Erics-Macbook grape % git checkout master
Switched to branch 'master'
eproulx@Erics-Macbook grape % ruby benchmark/simple.rb
Warming up --------------------------------------
              simple   522.000  i/100ms
Calculating -------------------------------------
              simple      5.420k (± 5.8%) i/s -     27.144k in   5.025995s
eproulx@Erics-Macbook grape % ruby benchmark/simple.rb
Warming up --------------------------------------
              simple   531.000  i/100ms
Calculating -------------------------------------
              simple      5.490k (± 4.7%) i/s -     27.612k in   5.041801s
eproulx@Erics-Macbook grape % git checkout lazy_lookup_lazy_object
Switched to branch 'lazy_lookup_lazy_object'
eproulx@Erics-Macbook grape % ruby benchmark/simple.rb            
Warming up --------------------------------------
              simple   605.000  i/100ms
Calculating -------------------------------------
              simple      5.871k (±10.0%) i/s -     29.040k in   5.000857s
eproulx@Erics-Macbook grape % ruby benchmark/simple.rb
Warming up --------------------------------------
              simple   606.000  i/100ms
Calculating -------------------------------------
              simple      6.409k (± 4.0%) i/s -     32.118k in   5.019708s

Thanks

@grape-bot
Copy link

grape-bot commented Feb 29, 2020

1 Warning
⚠️ There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

@ericproulx ericproulx force-pushed the lazy_lookup_lazy_object branch from 4b9a313 to dca18e7 Compare February 29, 2020 18:06
@@ -160,7 +169,7 @@ def greedy_match?(input)
end

def call_with_allow_headers(env, methods, endpoint)
env[Grape::Env::GRAPE_ALLOWED_METHODS] = methods.join(', ')
env[Grape::Env::GRAPE_ALLOWED_METHODS] = -methods.join(', ')
Copy link
Member

Choose a reason for hiding this comment

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

explain?

Copy link
Contributor Author

@ericproulx ericproulx Mar 1, 2020

Choose a reason for hiding this comment

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

For those who are using ruby >= 2.5
https://rubyreferences.github.io/rubychanges/2.5.html#string--optimized-for-memory-preserving

I guess I could have simply put .freeze instead of -. freeze already does it even in 2.4

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Let's use .freeze until we change it everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it

@dblock dblock merged commit 0022804 into ruby-grape:master Mar 1, 2020
@dblock
Copy link
Member

dblock commented Mar 1, 2020

I merged this. LGTM.

fcheung added a commit to dressipi/grape that referenced this pull request Jul 10, 2020
This optimisation was introduced in ruby-grape#2002. If an api's paths include
an ids in the path then this cache can grow in an unbounded manner
fcheung added a commit to dressipi/grape that referenced this pull request Jul 10, 2020
This optimisation was introduced in ruby-grape#2002. If an api's paths include
an id in the path then this cache can grow in an unbounded manner.
fcheung added a commit to dressipi/grape that referenced this pull request Jul 10, 2020
This optimisation was introduced in ruby-grape#2002. If an api's paths include
an id in the path then this cache can grow in an unbounded manner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants