Fix performance issues with JRuby

[guizmaii]

Fix #640

[delner]

It's hard to say for sure, but your tests may be failing in tracer_activerecord_test.rb:86 and tracer_activerecord_test.rb:145 for a few reasons:

1. The spans appeared in a different order than expected, and the test accidentally swapped the sinatra span for the sqlite span. This is more because of a brittle test than a code flaw. You could fix this by changing the test to assign the correct spans to sqlite_span and other variables correctly.
2. The test did not generate the correct number of spans, which shifted variable assignment. This would be because of a code flaw.
3. The test generated the correct spans and assignments, but spans actually have the wrong service names. This would be the result of a code flaw, perhaps because ActiveRecord is not calculating the correct service name from the adapter.

I'd check each of those, see if that helps fix things.

[delner]

I really like the simple approach here, and what you have should be backwards compatible from what I can tell. Left a question regarding the 'connection search' logic you had.

Although I'm not a fan of having to search through Rails connections every time an event is done, the Rails guys said it's unlikely to have a big performance impact. We'll probably want to keep an eye on this to verify this is okay. At least we're getting rid of _id2ref so that's a plus.

Let's try to get these tests passing. I think we can give it one last review and probably merge it if we can.

[delner]

Will this iterate over all ActiveRecord connections? Regardless if they were made with ActiveRecord::Base.establish_connection, or from a class that inherits from ActiveRecord::Base? Just want to make sure connections configured in a legitimate way for Rails don't slip through the cracks.

[kares]

yep all there is connected (in whatever way) with AR

[guizmaii]

I think, but I'm not sure, that it's all connections in the current process, because here's how connection_pool_list is defined (at least in Rails 4.2):

      def connection_pool_list
        owner_to_pool.values.compact
      end

and here's how owner_to_pool is defined:

      def owner_to_pool
        @owner_to_pool[Process.pid]
      end

If I understand well (and I'm absolutely not sure of that) this line of code: @owner_to_pool[Process.pid], it says that it takes the pools for the current process only.

Which means that for a multi-processes runtime (like Puma with workers or Unicorn) the number of connections should be fairly low (at least, it's my assumption. I know that AR is by default configured to use 5 connections, IIRC)
And for a multi-threaded runtime (like Puma with JRuby) it means all the connections but it seems that in this case AR uses only one connection per thread (maybe per db) which is again fairly small (I think 😕).

WDYT ?

[kares]

you got that right: one connection per thread
per Model.establish_connection (class_to_pool) - which usually means a different DB connection.

Process.pid is really only due MRI - not much forking under JRuby.
under a multi-process (MRI) connections are low (usually ~1 since one process == one request), under a GIL-less runtime it can go as high as the server handles concurrent requests.

[delner]

@kares Clever that since its segmented by process/thread that it makes the pool to be searched much smaller. I think this is only problematic if the AR event is generated from one thread, and handled in another, which would cause it to search the wrong pool. I can't really think of how that would happen here.

@guizmaii Okay, so I guess my last concern is, does this work with Rails 3.0 - latest? We have broken tests in this PR so its hard to tell. If we can get the build passing, it will run the Rails tests for these different versions, and that would be good enough to tell me whether this condition has been satisfied.

[delner]

I think I prefer your implementation here, but just for reference, I did open a couple of branches that's an alternative implementation of this.

• This branch adds a "shim" which allows hooks to be inserted around other objects.
• This branch uses that "shim" to inject :connection into the ActiveRecord event for Rails versions that don't already have it. It then uses the :connection object off that event just like yours.

It might be a good idea to compare the performance of each of these, see if there's a significant difference or not. But I'm tempted to keep yours if it's similar or better in performance since what you have is simpler. For now, let's get your tests passing.

[guizmaii]

@delner

I still have to look at your alternative solutions but the second link you propose redirect to this issue. IMO, it's not the correct link ;)

[delner]

@guizmaii Whoops, forgot to fill in the link. Fixed now.

[guizmaii]

Closed in favor of #649

[delner]

@guizmaii Per #661 we should probably re-open this and implement it over what I had in #649. That PR works by itself, but it has too many incompatibilities with 3rd party libs that serialize sql.active_record payload. I think this one will be likely more compatible since it doesn't interfere with serialization like the other did.

[delner]

I couldn't push to this branch (permission was denied) so I opened up #662 instead.