intridea/omniauth — pull request #773 by schneems (2014-09-29 15:55:35+00:00)
Title: Hashie Breakup - Major Perf boost

Hashie, we've had some great times...but I think we need to break up. Your ties to omniauth are expensive, you're too high maintenance. We needed to take it to the next level but things aren't moving fast enough. It's just not working out...

I benched a Rails request with [memory_profiler](https://github.com/SamSaffron/memory_profiler) and found that Hashie allocates a TON of objects on each request:

```
allocated memory by gem
-----------------------------------
rack-1.5.2 x 6491
actionpack-4.1.2 x 4888
hashie-3.3.1 x 4735 <==============
activesupport-4.1.2 x 3292
omniauth-1.2.2 x 1387
actionview-4.1.2 x 1107
ruby-2.1.2/lib x 1097
railties-4.1.2 x 925
activerecord-4.1.2 x 440
warden-1.2.3 x 200
codetriage-ko1-test-app/app x 40
other x 40
```

This is one, request! I was curious if it was possible to remove hashie from Omniauth, and if so, would the result be better, faster, and use less memory. This PR does just that.

## Patch Implementation

This patch removes Hashie as a dependency and replaces its usage with a custom data structure `OmniStruct` based off of `OpenStruct`. I think we can even remove this structure eventually in favor of some honest to goodness PORO configuration options, but we will have to deprecate some interfaces and rev versions so...one step at a time.

## Patch Benchmarks

It uses less memory:

```
allocated memory by gem
-----------------------------------
rack-1.5.2 x 6491
actionpack-4.1.2 x 4888
activesupport-4.1.2 x 3292
ruby-2.1.2/lib x 1337 <=========
omniauth/lib x 1267
actionview-4.1.2 x 1107
railties-4.1.2 x 925
activerecord-4.1.2 x 440
warden-1.2.3 x 200
codetriage-ko1-test-app/app x 40
other x 40
```

Our structure uses `4735 - 1337 # => 3398` fewer objects (per request 0_o). I benchmarked a request with and without the patch and this was the result.

Without patch:

```

Calculating ——————————————————
ips 54 i/100ms
-------------------------------------------------
ips 547.3 (±4.2%) i/s - 2754 in 5.041327s
```

With patch:

```

Calculating ——————————————————
ips 55 i/100ms
-------------------------------------------------
ips 576.7 (±4.2%) i/s - 2915 in 5.063895s
```

Our patch improves speed by:

```
(576.7 - 547.3)/547.3 * 100
# => 5.37 # %
```

This isn't `5%` speed increase in Omniauth, this is a `5%` speed increase in the __whole rails app request__.

## Implementation considerations

Most tests pass with no modification and most interfaces remain unchanged. The biggest change is that there are no more predicate methods so use `skip_info` instead of `skip_info?`. These were never explicitly tested. Also, if we move to proper configuration objects we can put these back in where they make sense, some custom defined predicates still remain such as `valid?` on `AuthHash because they were manually defined.

There was one test that needed a `to_hash` called on them because of the recursive nature of the OmniStruct (if you set a value as a hash, it will store it as an OmniStruct so we can do method call chaining). I think in this case the change is fine because the resultant object can be accessed the same way (keys and methods). This behavior was never explicitly tested for, only implicitly in this one test, which leads me to believe that it is an acceptable change.

## Now what?

The case for removing hashie is pretty compelling, let's :shipit: or discuss implementation. Afterwards we can talk about a plan to move omniauth towards real honest to goodness PORO config objects.