Lower the memory allocations #1188

stokarenko · 2019-03-10T13:15:07Z

It prevents the repeating hashes creation during the calculation of object and object_changes.

The real-life case, discovered with a help of memory_profiler gem:

It saves ~35Mb per bulk of 100 typical objects like User;
It saves ~875Mb per Sidekiq process full of bulk jobs like P1, under default concurrency 25.

As a part of RAM optimization we do:

memoization of repeatedly called PaperTrail::Events::Base methods;
cache the AR changed_attributes for AR < 5.1;
prefer to build the Version objects by pure Class.new instead of association.build.

The record's association cache will be invalidated on the way by association.reset, so no any explicit versions.reload calls are needed now.

Wrote good commit messages.
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new
code introduces user-observable changes.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.

jaredbeck

.

spec/paper_trail/cleaner_spec.rb

jaredbeck · 2019-03-11T18:56:57Z

It saves ~35Mb per bulk of 100 typical objects like User;

Awesome! Thanks for working on this.

jaredbeck · 2019-03-11T18:59:58Z

I'd expect a memory optimization to not change existing tests.

Said another way, I'd be a lot more comfortable reviewing this if existing tests can be kept exactly the same.

stokarenko · 2019-03-12T17:42:49Z

Reverted the changes on tests, @jaredbeck please revisit.

jaredbeck

Sergey, I've finished my first review. Generally speaking, I like the direction this is going.

In this first review, I've focused on getting more information about some things I don't understand, and trying to limit the scope. It's a big PR, so if you don't mind splitting it up a bit, as I suggest below, that would be very helpful.

In future reviews, I'll look more closely at some of the logic, and I'll be running your benchmarks locally.

lib/paper_trail/events/base.rb

lib/paper_trail/record_trail.rb

spec/paper_trail/association_reify_error_behaviour/error_spec.rb

jaredbeck · 2019-03-12T18:21:21Z

spec/paper_trail/model_spec.rb

+        expect(&version_building).to allocate_less_than(35).kilobytes
+      end
+    end
+  end


It's very cool that you were able to write these benchmarks as tests. I've never worked on a project with such tests, but I'm willing to try it. My only concern would be if, in the future, memory usage inside rails increases (in a way we cannot control) causing these tests to fail. If it becomes painful in the future, we may need to separate these benchmarks from the test suite.

Any optimization efforts become to be mindless without such kind of tests :)
Any next PR can easily ruin all the winnings.

As for the third-party changes, which are really out of our control - at least we will know about the problems with them. We can mark such particular gems and versions as ineffective both in readme and tests for example.

In general you're right, the life will be harder with these tests :)

jaredbeck · 2019-03-12T18:23:03Z

spec/support/performance_helpers.rb

+    "expected that example will allocate less than #{expected}#{@scale},"\
+    " but allocated #{@allocated}#{@scale}"
+  end
+end


I did not know the RSpec matcher API was so flexible. This is pretty great.

stokarenko · 2019-03-13T14:05:59Z

@jaredbeck please take a look at https://travis-ci.org/paper-trail-gem/paper_trail/builds/505655659

As you can see, the removal of #1189 is strongly affect to performance. I'm forced to make more pessimistic assertions for this PR.

stokarenko · 2019-03-13T15:23:01Z

@jaredbeck it is ready for the next review )

jaredbeck

Second review: Now that #1189 has been merged, please rebase and restore the memory numbers in the test.

There probably won't be a need for a third review, thank you for your work on this so far. Splitting it up into smaller pieces was very helpful, thanks.

lib/paper_trail/record_trail.rb

It prevents the repeating hashes creation during the calculation of `object` and `object_changes`. The real-life case, discovered with a help of `memory_profiler` gem: 1. It saves `~35Mb` per bulk of 100 typical objects like `User`; 2. It saves `~875Mb` per Sidekiq process full of bulk jobs like P1, under default concurrency 25.

stokarenko · 2019-03-14T14:06:07Z

please rebase and restore the memory numbers in the test.

done, please revisit )

Thank you for assistance during this journey )

jaredbeck · 2019-03-14T18:49:07Z

I don't want to make any changes to our public API as part of an optimization PR.

I'm going to make build_version_on_create and build_version_on_update private, if you can live with that. I want it to be clear that they are not part of our public API (yet).

If you'd like to discuss some ideas for the public API, please email me (jared@jaredbeck.com) as we have temporarily disabled GH issues.

jaredbeck · 2019-03-14T19:33:29Z

Released as 10.2.1

stokarenko force-pushed the lower-the-memory-allocations branch 3 times, most recently from 5782dba to 2d09f08 Compare March 10, 2019 14:08

jaredbeck reviewed Mar 11, 2019

View reviewed changes

spec/paper_trail/cleaner_spec.rb Outdated Show resolved Hide resolved

stokarenko force-pushed the lower-the-memory-allocations branch from 2d09f08 to a0eb617 Compare March 12, 2019 17:39

jaredbeck requested changes Mar 12, 2019

View reviewed changes

stokarenko force-pushed the lower-the-memory-allocations branch 2 times, most recently from 44474b2 to 6966f83 Compare March 13, 2019 09:17

stokarenko force-pushed the lower-the-memory-allocations branch from 6966f83 to 77e9174 Compare March 13, 2019 14:12

jaredbeck requested changes Mar 13, 2019

View reviewed changes

lib/paper_trail/record_trail.rb Show resolved Hide resolved

stokarenko force-pushed the lower-the-memory-allocations branch from 77e9174 to 770d673 Compare March 14, 2019 13:52

jaredbeck approved these changes Mar 14, 2019

View reviewed changes

jaredbeck merged commit cf17bc4 into paper-trail-gem:master Mar 14, 2019

jaredbeck added a commit that referenced this pull request Mar 14, 2019

Follow-up to PR #1188

43dca0b

aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020

Follow-up to PR paper-trail-gem#1188

e87e306

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lower the memory allocations #1188

Lower the memory allocations #1188

stokarenko commented Mar 10, 2019 •

edited

Loading

jaredbeck left a comment •

edited

Loading

jaredbeck commented Mar 11, 2019

jaredbeck commented Mar 11, 2019

stokarenko commented Mar 12, 2019

jaredbeck left a comment •

edited

Loading

jaredbeck Mar 12, 2019

stokarenko Mar 13, 2019

jaredbeck Mar 12, 2019

stokarenko commented Mar 13, 2019

stokarenko commented Mar 13, 2019

jaredbeck left a comment

stokarenko commented Mar 14, 2019

jaredbeck commented Mar 14, 2019

jaredbeck commented Mar 14, 2019

Lower the memory allocations #1188

Lower the memory allocations #1188

Conversation

stokarenko commented Mar 10, 2019 • edited Loading

jaredbeck left a comment • edited Loading

Choose a reason for hiding this comment

jaredbeck commented Mar 11, 2019

jaredbeck commented Mar 11, 2019

stokarenko commented Mar 12, 2019

jaredbeck left a comment • edited Loading

Choose a reason for hiding this comment

jaredbeck Mar 12, 2019

Choose a reason for hiding this comment

stokarenko Mar 13, 2019

Choose a reason for hiding this comment

jaredbeck Mar 12, 2019

Choose a reason for hiding this comment

stokarenko commented Mar 13, 2019

stokarenko commented Mar 13, 2019

jaredbeck left a comment

Choose a reason for hiding this comment

stokarenko commented Mar 14, 2019

jaredbeck commented Mar 14, 2019

jaredbeck commented Mar 14, 2019

stokarenko commented Mar 10, 2019 •

edited

Loading

jaredbeck left a comment •

edited

Loading

jaredbeck left a comment •

edited

Loading