mobility: Performance much slower in version 1.2.3

Performance appears to be much slower in version 1.2.3 than version 1.2.1

Context

I haven’t been able to track this down completely but I believe the performance issue is related to this PR as I can’t reproduce it on version 1.2.1 and we make extensive use of fallbacks.

I’ve tried to reproduce this with a vanilla Rails app and haven’t been able to get but wanted to get this over to you in case you had any ideas.

Holding everything else constant, just initializing an ActiveRecord instance that has translated attributes is orders of magnitude slower.

Version 1.2.3:

Benchmark.realtime { 1000.times { Thing.new.title } }

=> 85.30171160000464

Version 1.2.1:

Benchmark.realtime { 1000.times { Thing.new.title } }

=> 0.3301175000015064

Expected Behavior

Performance is not significantly worse in version 1.2.3 than 1.2.1

Actual Behavior

Performance is significantly worse in version 1.2.3 than 1.2.1

Possible Fix

Revert https://github.com/shioyama/mobility/pull/531

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Comments: 18 (18 by maintainers)

Most upvoted comments

@shioyama just confirming that performance looks much better now, thank you!

Before:

Benchmark.realtime { 1000.times { Thing.new.title } }

=> 85.30171160000464

After:

Benchmark.realtime { 1000.times { Thing.new.title } }

=> 0.6135440160287544

@f1sherman I released 1.2.4, when you have a chance can you check that the regression is fixed for you?

Ok sorry, the issue has nothing to do with fallbacks or the change. It’s related to https://github.com/shioyama/mobility/pull/536 which is on the master branch.

I’ll just rebased the change in #546 onto the 1-2 branch and ship it there, should be no issues.

The problem you’re seeing on master is because of the fix for this issue: https://github.com/shioyama/mobility/issues/535

And basically the solution is, yes, to set a default value for the jsonb column! But it will be released in 1.3 not a patch version, with a warning about it.

So this is all clear now, sorry @f1sherman I should have realized this.