Shopify / yjit-bench

Set of benchmarks for the YJIT CRuby JIT compiler and other Ruby implementations.
MIT License
87 stars 22 forks source link

Increase activerecord benchmark size #297

Closed casperisfine closed 6 months ago

casperisfine commented 6 months ago

The Active Record benchmark doesn't do a whole lot. It load a single record without any association and just read a few fields. It also always fetch directly from the database.

Here we add a single extra association, enable the query cache to remove IOs entirely, and access a few more attributes to exercise the casting code more

Before:

interp: ruby 3.3.1 (2024-04-23 revision c56cd86388) [arm64-darwin23]
yjit: ruby 3.3.1 (2024-04-23 revision c56cd86388) +YJIT [arm64-darwin23]

------------  -----------  ----------  ---------  ----------  ------------  -----------
bench         interp (ms)  stddev (%)  yjit (ms)  stddev (%)  yjit 1st itr  interp/yjit
activerecord  29.4         2.8         13.2       2.7         1.55          2.23
------------  -----------  ----------  ---------  ----------  ------------  -----------

After:

interp: ruby 3.3.1 (2024-04-23 revision c56cd86388) [arm64-darwin23]
yjit: ruby 3.3.1 (2024-04-23 revision c56cd86388) +YJIT [arm64-darwin23]

------------  -----------  ----------  ---------  ----------  ------------  -----------
bench         interp (ms)  stddev (%)  yjit (ms)  stddev (%)  yjit 1st itr  interp/yjit
activerecord  165.8        2.6         71.2       2.6         1.93          2.33
------------  -----------  ----------  ---------  ----------  ------------  -----------

Profile before:

Capture d’écran 2024-05-07 à 14 24 37

Profile after:

Capture d’écran 2024-05-07 à 14 24 49

You can see that before the top leafs were mundane things such as allocating record, Sqlite3 and accessing various thread local state. After the change the top elements are more relevant things such as attribute access and casting, association mapping, etc.

NB: Given that activerecord is marked as headline, and this is quite a radical change, perhaps it can just be a new benchmark, called activerecord2 or activerecord-read-heavy or something?

casperisfine commented 6 months ago

Alternatively, since Rails 7.2 should go out soon, we could wait and do the upgrade at the same time.

maximecb commented 6 months ago

Thanks Jean this looks a lot nicer. Let's merge it. We want the headline benchmarks to be reasonably fair/representative 👍

eregon commented 6 months ago

The change looks good to me. Two thoughts:

maximecb commented 6 months ago

Those are fair points.

I think it would make sense to generate a string as Benoit suggests.

Wrt query cache, ideally we'd generate a lot of posts (time budget < 10s to generate), and then request random posts. Some of them would get cached, some not.

casperisfine commented 6 months ago

in most Rails apps one would need to generate a string from these attributes

This is generally true. But that's more the job of "rails-bench" or similar to simulate a full Rails request. If you simulate templating, you might as well also do the controller etc etc.

Here the focus is Active Record itself. I think it make sense to only exercise Active Record APIs.

Using the query cache here doesn't seem representative

Similarly, IMO the goal is to show what's left to optimize. We can totally do the SQLite query I don't mind, but that's just gonna add some overhead that JITs can't do anything about. I guess it somewhat exercise C bindings, so why not.

Some of them would get cached, some not.

I'd be concerned it would increase variance between runs.

eregon commented 6 months ago

This is generally true. But that's more the job of "rails-bench" or similar to simulate a full Rails request. If you simulate templating, you might as well also do the controller etc etc.

Here the focus is Active Record itself. I think it make sense to only exercise Active Record APIs.

Right, agreed a controller etc is too much. OTOH I think it's not so rare that Active Record is used outside of Rails. And regardless where it's used, it's never going to query the database and do nothing with the data, at minimum there would be some processing on the data. Transforming to a string seems a very common processing step (also happens e.g. just when printing data).

I see it as like microbenchmark vs something representative of a real usage of ActiveRecord. Given this is not in microbenchmarks, I think a simple processing step is good.

Similarly, IMO the goal is to show what's left to optimize. We can totally do the SQLite query I don't mind, but that's just gonna add some overhead that JITs can't do anything about. I guess it somewhat exercise C bindings, so why not.

It's part of real usages of ActiveRecord so I wouldn't remove it. And if the JIT can optimize anything around calling C methods it might be very much worthwhile (also there might be things to optimize in the sqlite3 C ext). (FWIW TruffleRuby < 24.0 runs C exts on Sulong, so it JIT compiles C code)

Some of them would get cached, some not.

I'd be concerned it would increase variance between runs.

Also since the cache is global it would cause variance between iterations (not sure if that's what you meant by runs). I think in a well-behaved app the query cache gets only cache misses, or very few hits overall (compared to numbers of queries), and so it seems more representative to disable it, but correct me if I'm wrong.

maximecb commented 6 months ago

I think in a well-behaved app the query cache gets only cache misses, or very few hits overall (compared to numbers of queries), and so it seems more representative to disable it, but correct me if I'm wrong.

Is the cache part of sqlite itself? If so, the cache hit rate should actually be fairly high in a real server. I agree that there are concerns with variance. 🤔

What's the performance like with the cache on vs off?

With respect to building a string to output, I do think it might make some sense to do that. If there is no output, a sufficiently advanced JIT may be able to eliminate the calls.

eregon commented 6 months ago

Is the cache part of sqlite itself?

It's this: https://guides.rubyonrails.org/caching_with_rails.html#sql-caching and per request, so normally it wouldn't hit but because we don't really have a request here it becomes effectively a global cache.