[WIP] Fix performance issue when decorator files are loaded multiple times

[patcoll]

NOTE: This is a work-in-progress.

Fixes #28

Here is a proof-of-concept here that shows the slowdown:

https://github.com/luma-institute/roar-jsonapi-test

In our case, we use trailblazer and roar-jsonapi within Rails, and the performance slowdown showed up in our development environment. We were able to reproduce the problem without Rails being involved, both in the linked repo and with a minimal benchmark test case in this PR.

Failing test written first. Goal is to come up with a minimal fix to be applied to this repo to make the test pass and thus remove the slowdown.

[myabc]

@apotonick may be interested in taking a look at the patches: https://github.com/luma-institute/roar-jsonapi-test/tree/master/patches

[apotonick]

This is definitely a problem:

  assert_performance_constant do |n|
      n.times do |i|
        # simulates Rails auto-reloading
        load File.join($SAMPLES_PATH, 'simple_single_resource_object_decorator.rb')
      end

      SimpleSingleResourceObjectDecorator.new(Article.new(1, 'My Article')).to_json
    end

The representer is not designed to be loaded multiple times - this will of course lead to wrong behavior such as filling up a declarative variable defaults. Is this the problem? The variable should get flushed when unloaded in development. Can we simulate that somehow?

[apotonick]

Hey friends, I was playing a bit with this and came to the following conclusions.

1. Using load to "reload" a class will lead to the bug you're describing. Class instance variables will stick around and mess up after-reload state. See https://twitter.com/apotonick/status/872522867098996736
2. However, I wasn't able to provoke this in a Rails dev environment. It somehow manages to erase class instance variables and even with numerous experiments I couldn't "break" it.

[apotonick]

And:
3. I can confirm this bug appears with JSONAPI! It just can't be "fixed" with the load example even though it was an incredible help to find this bug.

[apotonick]

Update: The bug can also be provoked without the roar-jsonapi gem, which is "great" because it narrows down the possibilities.

Damn, someone pour me a scotch.

[apotonick]

Ok, the problem is that reloading in Ruby simply doesn't work.

I could replicate the problem without any Roar or Representable, just any inheritance/include is sufficient to deactivate Rails' "class cleansing" mechanism and suddenly class instance variables survive the reload. Note that in Ruby this is per design (or poor implementation), in Rails it's a misconception where autoloading sometimes cleans up, sometimes it doesn't.

Personally, I'm unsure about how to tackle this. I am not willing to design my libraries to work with Rails autoloading and/or poor pseudo reloading mechanism that never really work.

[dmuneras]

@apotonick thanks for taking the time to dig into this, this doesn't seems to be something easy to address. Hope we find way around this problem.

[apotonick]

I am pretty sure this should work as expected when sticking to the Rails file naming convention, which works with TRB, too. Any results on this?

[patcoll]

We took this code out of our critical path for our product work for now. I did change code in an internal PR to match the Rails file naming conventions and it seemed to help. Waiting on a code review to confirm with some other folks.

[dmuneras]

@apotonick just tested what @patcoll did and it works in our project. :)

[patcoll]

It's worth mentioning here that the fix we ended up using involved matching the class names to a standard Ruby file naming convention in a trailblazer-rails context.

I believe the following factors contributed to our successful fix:

• The representer class that used the include Roar::JSON::JSONAPI.resource API was namespaced in a way that matched the file path. (Our class name was Video::Representer::VideosRepresenter and the file path was app/concepts/video/representer/videos_representer.rb).
• We had the app/concepts directory added to Rails autoload_paths like so: config.autoload_paths << Rails.root.join('app/concepts')

With that configured, an attempt to use the Video::Representer::VideosRepresenter class would search the autoload paths for a file that matched that structure and would successfully find it, require it, and use it. Similarly, upon a change in that file, whatever Rails constant magic happens for auto-reloading would work.

[apotonick]

@myabc ☝️ Sounds good!?!

[patcoll]

It's also worth mentioning that we tested without app/concepts folder being added explicitly and it worked the same. Rails 5.0.1

[myabc]

@apotonick remind me of the question!