Perf: Improve performance of where when using an array of values

[eileencodes]

A coworker at GitHub found a few months back that if we used
santitize_sql over where when we knew the values going into where
it was a lot faster than where.

This PR adds a new Arel node type called HomogenousIn that will be
used when Rails knows the values are all homogenous and can therefore
pick a faster codepath. This new codepath skips some of the required
processing by where to make wheres with homogenous arrays faster
without requiring the application author to know when to use which query
type.

Using our benchmark code:

ids = (1..1000).each.map do |n|
  Post.create!.id
end

Benchmark.ips do |x|
  x.report("where with ids") do
    Post.where(id: ids).to_a
  end

  x.report("where with sanitize") do
    Post.where(ActiveRecord::Base.sanitize_sql(["id IN (?)", ids])).to_a
  end

  x.compare!
end

Before this PR comparing where with a list of IDs to sanitize sql:

Warming up --------------------------------------
      where with ids    11.000  i/100ms
 where with sanitize    17.000  i/100ms

Calculating -------------------------------------
      where with ids    115.733  (± 4.3%) i/s -    583.000  in   5.045828s
 where with sanitize    174.231  (± 4.0%) i/s -    884.000  in   5.081495s

Comparison:
 where with sanitize:      174.2 i/s
      where with ids:      115.7 i/s - 1.51x  slower

After this PR comparing where with a list of IDs to sanitize sql:

Warming up --------------------------------------
      where with ids    16.000  i/100ms
 where with sanitize    19.000  i/100ms

Calculating -------------------------------------
      where with ids    158.293  (± 6.3%) i/s -    800.000  in   5.072208s
 where with sanitize    169.141  (± 3.5%) i/s -    855.000  in   5.060878s

Comparison:
 where with sanitize:      169.1 i/s
      where with ids:      158.3 i/s - same-ish: difference falls within error

Notes:

1. sanitize2 is a horrific name, obviously. We aren't sure what to
   name it though. Thoughts?
2. HomogenousIn may not be the best name for our new node.

Co-authored-by: Aaron Patterson aaron.patterson@gmail.com

cc/ @tenderlove

[tenderlove]

I'm not sure that HomogenousIn is the best name because the collection that it wraps is not homogenous in the traditional sense. Not all types in the collection are the same, rather they are all safe to pass to connection quote, and they are all terminal nodes (IOW, the HomoegenousIn node cannot point to a subselect, where the regular In node can)

[tenderlove]

We should probably add a comment here about why we're doing this. This is a huge "hack" to check to see whether or not value is numeric. Non-numbers will return nil for <=>, and it avoid us need to do an is_a? check. We found using the spaceship operator was faster than checking types.

[kamipo]

I'm curious why not use value.is_a?(Numeric). <=> is faster alternative of value.is_a?(Numeric)?

[tenderlove]

The issue with the current serialize method is that it does both value validation and serialization (but only sometimes). We would like to split that functionality in to two methods for this patch.

Here is an example of what I mean. The active model integer type's serialize method (which can be found here) looks like this:

      def serialize(value)
        return if value.is_a?(::String) && non_numeric_string?(value)
        ensure_in_range(super)
      end

ensure_in_range will validate that the value is valid. If it isn't, it raises an exception. If no exception is raised, the call to super will actually serialize it and return the serialized form.

Another behavior that Active Record has is that we will omit values that are outside of range supported by the database. Here is an example:

class Post < ActiveRecord::Base
end

params = {}
params[:id] = 9999999999999999999999999999999

$-d = true
Post.where(id: [params[:id], 1234]).to_a

The SQL executed by the above code is:

SELECT `posts`.* FROM `posts` WHERE `posts`.`id` IN (1234)

Active Record strips out the very large number because the database can't handle it.

An unfortunate consequence of "raising + serializing" combined as one unit, and stripping out-of-range values is that we have to raise and catch exceptions in order to support this. If you run the above example, you'll actually see that an ActiveModel::RangeError is raised in the course of running the program on master (this branch does not):

Exception `ActiveModel::RangeError' at /Users/aaron/git/rails/activemodel/lib/active_model/type/integer.rb:40 - 9999999999999999999999999999999 is out of range for ActiveModel::Type::Integer with limit 8 bytes
D, [2020-04-21T14:38:37.905285 #78972] DEBUG -- :   Post Load (0.5ms)  SELECT `posts`.* FROM `posts` WHERE `posts`.`id` IN (1234)

It would be nice if we could check that a value is valid without having to serialize it, only to get an exception.

Unfortunately the serialize method in Active Model is public, so we don't want to change the behavior. Is there a different name we could use?

Second, this "raise if not valid" behavior is not consistent among all types. For example, the Enum type will not raise an exception on invalid values.

I think serializable? is a good method that should be in Active Model. But I don't know what to do about serialize vs serialize2.

[tenderlove]

Suggested change

predicate_builder.connection.in_clause_length

I think we can remove this.

[tenderlove]

I'm not sure that HomogenousIn is the best name because the collection that it wraps is not homogenous in the traditional sense. Not all types in the collection are the same, rather they are all safe to pass to connection quote, and they are all terminal nodes (IOW, the HomoegenousIn node cannot point to a subselect, where the regular In node can)

I was thinking about this a little more and I want to take back my statement. We know the list is homogenous (or must be homogenous) in the "traditional sense". All the values must have the same type. This line proves it. They all must be the same type so we can use the same type caster.

We should probably push this logic down in to the HomogenousIn node.

[rafaelfranca]

Why serialize2 exists? All definitions of it in this PR seems to have the same code as serialize.

[eileencodes]

It is used here https://github.com/rails/rails/pull/39009/files#diff-3bcf02e80bc52706c3df424d2a8c4db2R31 and it's a terrible name but we're not sure what to call it. The other serialize methods do both serialize and validation, this one just does serialization.

[rafaelfranca]

But if all the implementations of serialize2 on this PR is always the same as the implementation as serialize I don't see the different and serialize2 is also doing serialization and validation, no?

[tenderlove]

It's not the same. Check the serialize method on Integer here. It's doing both serialization and validation. This one is only doing serialization.

[jonathanhefner]

Is the role of serialize2 to be a fast, unchecked serialize (i.e. "garbage in, garbage out")? If so, would unchecked_serialize be an appropriate name?

[rafaelfranca]

Sorry, that was the context I was missing. In this diff all serialize2 methods have the same implementation of the serialize in the same files, but I was missing that the subclasses were overriding the method.

If we want to serialize2 to be part of the public API I'd go with @jonathanhefner's suggestion, but with the long term goal of renaming it to serialize and renaming the current serialize to something as verify_and_serialize.

[tenderlove]

@rafaelfranca I like @jonathanhefner's suggestion too. @eileencodes wdyt?

[eileencodes]

Works for me!

[rafaelfranca]

Should this be part of the public API?

[tenderlove]

I think it should. I would like it if active model types can tell us whether or not a value can be serialized for that particular type. We should add some docs, but I didn't want to go that far unless everyone was OK with adding this to the public API

[eileencodes]

We fixed the method names, made some improvements, and manually merged this in 70ddb8a.