[PATCH] Rewrite of filter.rb as promised. All tests pass and all documentation added.

Filter Overhaul with tests

Awaiting performance-impact analysis from Stefan.

I've measured the performance impact on both Lunix 64bit and WindowsXP 32bit, same machine.

On Linux, there's a noticable but not very big difference (3% to 10%), but on windows, I've measured differences of up to 40%.

Currently I don't know why there's such a big difference. I've never encountered a similar situation before during my Rails performance work.

I think this needs a little more investigation.

Here's the data obtained from running railsbench with my usual benchmarks:

perf data file 1: perf_run1.all
  requests=1000, options=-bm=all -mysql_session -patched_gc -fast_readers -custom_finders -dyn_piggy -builder -lib=trunk

perf data file 2: perf_run2.all
  requests=1000, options=-bm=all -mysql_session -patched_gc -fast_readers -custom_finders -dyn_piggy -builder -lib=trunk_new_filters

page   c1 real   c2 real  c1 r/s  c2 r/s  c1 ms/r  c2 ms/r  c1/c2
 1:    2.24467   2.35967   445.5   423.8     2.24     2.36   0.95
 2:    2.75033   3.50500   363.6   285.3     2.75     3.51   0.78
 3:    3.54200   4.95833   282.3   201.7     3.54     4.96   0.71
 4:    3.56767   6.20833   280.3   161.1     3.57     6.21   0.57
 5:    8.74467  11.27100   114.4    88.7     8.74    11.27   0.78
 6:    7.79700   8.61433   128.3   116.1     7.80     8.61   0.91
 7:    8.48933   8.76033   117.8   114.2     8.49     8.76   0.97
 8:    7.67200   7.88533   130.3   126.8     7.67     7.89   0.97

all:  44.80767  53.56233   178.5   149.4     5.60     6.70   0.84

GC:    c1 real   c2 real  c1 #gc  c2 #gc   c1 gc%   c2 gc%  c1/c2
       3.68667   4.20267    23.0    24.0     8.23     7.85   0.96

I've been thinking about this patch a little more, and I think I may have an idea for a way to make it a little faster (possibly). I'm not sure if it will make an effect or not, but it would remove the Filter class and simply add filters that are yielding filters directly to the chain, thereby reducing the number of objects created for the each filter chain.

One side effect of the speed increase I'm considering is that it removes some of the abilities to add things like extra properties or methods on Filter objects that make them more intelligent, or other things like naming a filter so that even proc/class filters could be named and skipped. However, those features may not be all that necessary when considering the speed of execution.

If there are any other ideas for speed improvements, I'd be happy to implement them and post another patch for testing.

Perhaps the filter chain traversal is less efficient now - you could profile the old and new code to see where their call graphs differ. This is a great patch btw, thanks :)

This will hopefully speed it up a bit. Uses indexes and doesn't pass filter arrays around. Should speed up execution time (I'm assuming).

Is there a way I could do the performance testing myself. I'm not quite sure what is being done to test this patch. Only way I know of currently is to make a controller with a bunch of filters and use Benchmark to see how long it takes to call an action. I'm not sure if it's very accurate though. One pass across many filters is much less an issue than many passes against 3-6 filters.

Any suggestions would be helpful and would also allow me to get this as good as it possibly can be.

zraii, I'd setup a test with a few different scenarios where you compare before the past to after the patch. Then post the results here.

I'd suggest to use my railsbench package to measure before/after, where before is edgde rails without any patch and after is the version obtained by applying your patch.

I'll measure my app again tomorrow.

Ok, I've reorganized the filter code in such a way that it will raise an error on bad filter type immediately when it's assigned in the class. This makes one of the tests unavoidably fail.

test/controller/filters_test.rb:202

before_filter 2

My code now gets:

./test/controller/../../lib/action_controller/filters.rb:554:in `class_for_filter': A filters must be a Symbol, Proc, Method, or object responding to filter. (ActionController::ActionControllerError)

In the patch I'm about to attach, this test and the before_filter 2 are commented. Please let me know if we need to do something better.

I think this new copy of the patch should further increase the speed of the filters. I removed almost all runtime logic from filter execution (as much as was possible). Filter type is now determined at controller class creation and the filters are built so as to know their type and not check filter types each time.

Ok, here's a new copy of the improved code. I can't see this not being an improvement. (And if not I'm not sure what else to try)

Sounds good.

Whoops, last patch I sent didn't include the yielding_around_filter_test.rb file. Should be able to get it from the previous diff or I'll attach it a little later when I have a minute.

Here's the test file. Mostly a direct translation of the meantime_filter tests by Roman (used with his permission)

When you're testing this with railsbench are you setting up an app for each of the trunks or are you able to just run it on the svn trunk alone somehow? Just trying to figure out how to do this benchmarking myself so I can help this move along.

Replying to martin.emde@gmail.com:

When you're testing this with railsbench are you setting up an app for each of the trunks or are you able to just run it on the svn trunk alone somehow? Just trying to figure out how to do this benchmarking myself so I can help this move along.

I'm not setting up a separate app. Instead I keep all my svn checkouts in one directory and use a modified boot.rb file to set up RAILS_FRAMEWORK_ROOT from a command line parameter passed to railsbench.

You can compare the performance of two different rails versions like so:

perf_diff 1000 "-bm=<your_benchmark_name>" "-lib=trunk" "-lib=trunk_with_patched_filters"

assuming your svn checkout directory uses trunk for the edge rails version and trunk_with_patched_filters for edge rails with your patch applied. I'll attach the modified boot.rb.

Don't get fooled by the statement that you shouldn't modify boot.rb. It's the best way to do it.

boot.rb modified to make it possible toeasily test your app with different rails svn checkouts

slightly improved version of the patch

I've slightly rewritten the patch. It's still a little slower than the original filter implementation (measured on 64bit Linux):

perf data file 1: perf_run1.all
  requests=1000, options=-bm=all -mysql_session -patched_gc -custom_finders -fast_readers -dyn
piggy -builder -OT -lib=trunk

perf data file 2: perf_run2.all
  requests=1000, options=-bm=all -mysql_session -patched_gc -custom_finders -fast_readers -dyn
piggy -builder -OT -lib=trunk_new_filters

page   c1 real   c2 real  c1 r/s  c2 r/s  c1 ms/r  c2 ms/r  c1/c2
 1:    1.68246   1.62828   594.4   614.1     1.68     1.63   1.03
 2:    1.91257   1.99986   522.9   500.0     1.91     2.00   0.96
 3:    1.98503   2.05176   503.8   487.4     1.99     2.05   0.97
 4:    1.99378   2.06091   501.6   485.2     1.99     2.06   0.97
 5:    4.55479   4.57785   219.5   218.4     4.55     4.58   0.99
 6:    4.96137   4.98863   201.6   200.5     4.96     4.99   0.99
 7:    5.13610   5.16042   194.7   193.8     5.14     5.16   1.00
 8:    4.96677   4.97483   201.3   201.0     4.97     4.97   1.00

all:  27.19287  27.44254   294.2   291.5     3.40     3.43   0.99

GC:    c1 real   c2 real  c1 #gc  c2 #gc   c1 gc%   c2 gc%  c1/c2
       1.92420   2.00035    22.0    22.0     7.08     7.29   1.00

But maybe this slight slowdown is acceptable.

I think that's looking really good. Especially considering we increased the complexity of the filter process a little.

I'm not sure if it would really increase the speed at all but you could run a quick reject on the filter_chain to eliminate excluded actions before hand, then you can skip the excluded_from? during the execution. I can't see it hurting it atleast.

something like:

    @before_filter_chain_aborted = (call_filter(self.class.filter_chain.reject { |f| f.excluded_from?(action_name) }, 0) == false)

Otherwise, thanks for all your help skaes and everybody and I hope this is acceptable for 1.2. I'm in the process of writing an article describing the new functionality and I'll give you a link to it asap. Maybe it can make it's way onto the Riding Rails weblog. :)

(In [5163]) Filters overhaul including meantime filter support for around filters. Closes #5949.

Please see the skip filter failures in #6297 and #6299.

Ok, so the inheritable attributes aren't being copied properly when new derived classes are being instantiated. Looks like we might need to switch to keeping the exclusion in the controller.

I'll write a couple test cases and post a patch so we can see this happening. I'll see that this gets fixed asap.

I can't get a test to fail on the new filters that doesn't fail on the old version as well. The one posted in Ticket #6299 fails on the old version for me.

I agree that this is improper behaviour but deep copy issues on the included_actions and excluded_actions inheritable attributes seem to prevent any clean solution.

If the included_actions is hash of filters as keys indexing an array of actions, the inheritable attributes #dup will only copy the location of that array, and use the same array data in memory. I can't see this ever having worked and I'm not seeing a clear solution without a change in the way these work. You can even see in the old code how we would rely on the same data set when removing excluded actions in a skip:

        def remove_contradicting_conditions!(filters, conditions)
          return unless conditions[:only]
          filters.each do |filter|
            next unless included_actions_for_filter = (read_inheritable_attribute('included_actions') || {})[filter]
            [*conditions[:only]].each do |conditional_action|
              conditional_action = conditional_action.to_s
              included_actions_for_filter.delete(conditional_action) if included_actions_for_filter.include?(conditional_action)
            end
          end
        end

You can see here that we're modifying the included_ations_for_filter array without any regard for inheritable attributes being kept for individual controllers.

Can someone else evaluate this test case and let me know if it looks like it should pass? If it looks good then I can try to get this feature to work. As far as I can tell (and these tests show), this feature never worked.

Test cases included from Ticket #6299 - These should work but no version has ever done this properly

Interesting... I assumed my #6299 issues were introduced in r5163 because my tests started breaking in r5163. But you're right, the tests in #6299 fail under r5162, too....

I can't figure out why my tests (application tests, rather than rails tests) work fine in r5162. I'll investigate further this weekend, if I get the chance.

What are the deep copy issues? This is the common usage for skip filters; it shouldn't be hard to reproduce.

Replying to bitsweat:

What are the deep copy issues? This is the common usage for skip filters; it shouldn't be hard to reproduce.

The arrays of actions (included_actions and excluded_actions) are not being copied when the inheritable hash is copied.

The problem with the pre r5163 method is that when the inheritable hashes are copied, they do not copy the arrays of actions that are their values. Then when we go to modify the array of actions it's still the same array for all the classes. There is never a point at which these arrays are copied so that they could actually hold different actions for each controller class.

for example:

h = { :a => [1,2] }
d = h.dup
d[:a].delete(2)
h[:a] => [1]

This is how the inheritable attributes work. This is also how actions are removed from included_actions in the old code. The new code also applies changes to all classes in much the same way.

I've been messing around with this all day and I don't see a simple way to have a hash with filters as keys pointing to any sort of array or hash that contains actions that should or should not run the filters. It's too many levels of copy depth to duplicate properly.

Just to update. I'm going to instead work on a couple of my own test cases based on the other ticket on this since the original test case we had didn't pass on either. We'll worry about that later.

Ok. I got one of those random "oh, all my tests passed.. how'd that happen?" moments. This appears to be working beautifully now. The tests include the example from #6299 and they pass. I'm going to be looking into _why_ they passed but atleast we can get the fix added that works.

Tests included from #6299. All passing and fix looks to be working great

Nice work. I've added the change into my app and it all seems to work perfectly.

Seems to be working here too. Thanks

Perhaps [5218] fixes the original problem. The fix here looks like a substantial implementation change.

That change to inheritable attributes was something that I had wanted to do for a while. I think it could be made to fix the problem if we override dup in the Filter class so it also dups it's included and excluded actions. Otherwise, the included/excluded actions will continue to be the same array and will reflect changes as such.

The "implementation change" is really just a switch back to the old style of storing included and excluded actions. I didn't think the new way would actually work without deeper copying in the inheritable attributes.

Ok, so I know a way that should work to take advantage of the deeper duplication with the new inheritable attributes. It will make a class FilterChain < Array which, when duped, will call dup on the Filters it holds. They will then each dup their included/excluded actions arrays. This should work to make sure that everything is dup'd properly when the controllers are inherited.

My question is this: This is another significant change of implementation. It may be for the better (in fact this was the original way I implemented this filter code before finding out that inheritable_attributes don't deep copy) but I wanted to see what others thought before I spent too much time on it.

I'll apply your fix using the old style of storing included and excluded actions. Doing a deep copy seems more natural, but the existing patch passes tests :)

(In [5268]) r5540@ks: jeremy | 2006-10-08 23:05:30 -0700

#5949 r5541@ks: jeremy | 2006-10-08 23:07:08 -0700 Fix filter skipping in controller subclasses. r5557@ks: jeremy | 2006-10-08 23:11:24 -0700 Update changelog. Closes #5949, references #6297, references #6299.

One last update: this patch broke the following scenario, where before_filter is used as a kind of skip_filter:

class A < AC::Base
  before_filter :authorize

  def show
  end
end

class B < A
  before_filter :authorize, :except => :index

  def index
  end
end

I just committed [5301], which adds support for that scenario back in. This was not caught before because there was insufficient test coverage of the original implementation. :(

Change set [5301] is incomplete and will not fix that bug entirely. It only affects the append filter methods. This action should be done when the filter is added and the conditions are decided, not at assignment.

the changes also reimplement the search mechanism for finding existing filters so we now have two entry points for finding a filter. Any changes to the way that works and we may have some odd bugs to work out. I would be happy to write a patch to take care of the loose ends this left. Would core like a patch against the existing trunk or against a pre [5301]?

I'm actually okay with this only working on append. However, if you want to rethink the search mechanism, that's cool, too. A patch against trunk would be ideal.

Applies to [5300] *not* [5301] Please use the same tests from 5301

ok, fix posted, I put it against [5300] since there was a bunch of stuff added in [5301] that was duplicate.

Any word on this making it in. The fix that was originally posted is not effective and reimplements too much code that already exists in the filter code. My new patch is basically a one line change from creating the filter to finding or creating the filter. I moved a few things around just to keep the code clean but this one change is all it needed.

Martin, I'd like very much to apply your patch, but I don't want to have to do a partial rollback of the existing code to make it happen. Could you rework your patch against trunk so that I can just apply it directly?

New version applied to trunk

Ok, thanks for your time :)

(In [5331]) More consistent implementation of filter replacement (thanks Martin! closes #5949)