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

Minimize object creation on middleware call. #774

Merged
merged 1 commit into from Oct 1, 2014

Conversation

Projects
None yet
2 participants
@schneems
Contributor

schneems commented Oct 1, 2014

This patch isn't nearly as invasive as #773. Also it was tested against itself, not in a full Rails app. I'm contributing the perf scripts I wrote in another PR.

Notice that by memoizing methods, not only are we reducing objects created in omniauth, but also in hashie due to fewer method calls to various omniauth options.

Bench

Before patch

Calculating -------------------------------------
                 ips       470 i/100ms
-------------------------------------------------
                 ips     4877.1 (±10.1%) i/s -      24440 in   5.061663s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       467 i/100ms
-------------------------------------------------
                 ips     4840.9 (±14.5%) i/s -      23817 in   5.068649s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       465 i/100ms
-------------------------------------------------
                 ips     4799.5 (±11.8%) i/s -      23715 in   5.019191s

After Patch

$ be rake perf:ips
Calculating -------------------------------------
                 ips       631 i/100ms
-------------------------------------------------
                 ips     6269.1 (±11.8%) i/s -      30919 in   5.014849s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       615 i/100ms
-------------------------------------------------
                 ips     6207.8 (±10.3%) i/s -      31365 in   5.106172s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       522 i/100ms
-------------------------------------------------
                 ips     6155.8 (±9.3%) i/s -      30798 in   5.048455s

Memory Bench

100 requests without patch

allocated objects by gem
-----------------------------------
hashie-3.3.1 x 6901
rack-1.5.2 x 5300
omniauth/lib x 3637 <====================
ruby-2.1.2/lib x 301
other x 300

100 requests with patch

allocated objects by gem
-----------------------------------
rack-1.5.2 x 4800
hashie-3.3.1 x 4701
omniauth/lib x 1037 <====================
ruby-2.1.2/lib x 301
other x 300

Results in (3637 - 1037 )/ 3637.0 * 100 # => 71 % less objects.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 1, 2014

Contributor

Tests are failing on a line I didn't touch. From rubocop:

lib/omniauth.rb:139:5: C: Outdent access modifiers like module_function.
    module_function
Contributor

schneems commented Oct 1, 2014

Tests are failing on a line I didn't touch. From rubocop:

lib/omniauth.rb:139:5: C: Outdent access modifiers like module_function.
    module_function
@mbleigh

This comment has been minimized.

Show comment
Hide comment
@mbleigh

mbleigh Oct 1, 2014

Contributor

👍 thanks for this. I was thinking to myself "there has to be a less-invasive way to get most of the perf gain from this" and voila! Open source in action. Let me see if I can track down the Rubocop bug and give you a fresh master against which to rebase.

Contributor

mbleigh commented Oct 1, 2014

👍 thanks for this. I was thinking to myself "there has to be a less-invasive way to get most of the perf gain from this" and voila! Open source in action. Let me see if I can track down the Rubocop bug and give you a fresh master against which to rebase.

@mbleigh

This comment has been minimized.

Show comment
Hide comment
@mbleigh

mbleigh Oct 1, 2014

Contributor

@schneems try rebasing on master and see if everything's kosher.

Contributor

mbleigh commented Oct 1, 2014

@schneems try rebasing on master and see if everything's kosher.

Minimize object creation on middleware call.
This patch isn't nearly as invasive as #773. Also it was tested against itself, not in a full Rails app. I'm contributing the perf scripts I wrote in another PR.

Notice that by memorizing methods, not only are we reducing objects created in omniauth, but also in hashie due to fewer method calls to various omniauth options.


## Bench

Before patch

```
Calculating -------------------------------------
                 ips       470 i/100ms
-------------------------------------------------
                 ips     4877.1 (±10.1%) i/s -      24440 in   5.061663s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       467 i/100ms
-------------------------------------------------
                 ips     4840.9 (±14.5%) i/s -      23817 in   5.068649s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       465 i/100ms
-------------------------------------------------
                 ips     4799.5 (±11.8%) i/s -      23715 in   5.019191s
```

After Patch

```
$ be rake perf:ips
Calculating -------------------------------------
                 ips       631 i/100ms
-------------------------------------------------
                 ips     6269.1 (±11.8%) i/s -      30919 in   5.014849s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       615 i/100ms
-------------------------------------------------
                 ips     6207.8 (±10.3%) i/s -      31365 in   5.106172s
2.1.2  ~/documents/projects/omniauth (master)
$ be rake perf:ips
Calculating -------------------------------------
                 ips       522 i/100ms
-------------------------------------------------
                 ips     6155.8 (±9.3%) i/s -      30798 in   5.048455s
```



## Memory Bench

100 requests without patch


```
allocated objects by gem
-----------------------------------
hashie-3.3.1 x 6901
rack-1.5.2 x 5300
omniauth/lib x 3637 <====================
ruby-2.1.2/lib x 301
other x 300
```

100 requests with patch 

```
allocated objects by gem
-----------------------------------
rack-1.5.2 x 4800
hashie-3.3.1 x 4701
omniauth/lib x 1037 <====================
ruby-2.1.2/lib x 301
other x 300
```

Results in `(3637  - 1037 )/ 3637.0 * 100  # => 71 %` less objects.
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 1, 2014

Contributor

Tests are passing thanks for the quick response!

Contributor

schneems commented Oct 1, 2014

Tests are passing thanks for the quick response!

mbleigh added a commit that referenced this pull request Oct 1, 2014

Merge pull request #774 from schneems/schneems/memoize-omniauth
Minimize object creation on middleware call.

@mbleigh mbleigh merged commit ed1f9a8 into omniauth:master Oct 1, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

rymai pushed a commit to gitlabhq/gitlabhq that referenced this pull request Feb 17, 2016

Merge branch 'update-omniauth-1-3-1' into 'master'
Update omniauth to 1.3.1 for memory performance

OmniAuth 1.3.1 may improve memory performance (~ 70% according to [intridea/omniauth!774](omniauth/omniauth#774)).

Closes #2404.

See merge request !2805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment