Skip to content

Refactor LocalCache to avoid calling Marshal.dump as much#42014

Merged
byroot merged 1 commit into
rails:mainfrom
Shopify:local-cache-rafactor-take-2
Apr 20, 2021
Merged

Refactor LocalCache to avoid calling Marshal.dump as much#42014
byroot merged 1 commit into
rails:mainfrom
Shopify:local-cache-rafactor-take-2

Conversation

@casperisfine

@casperisfine casperisfine commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

Second attempt at: #41882

The local cache need to protect against mutations of both the initial received value, and the value returned.

See:

Because of this, the overhead of the local cache end up being quite significant. On a single read, the value will be deep duped twice. So unless the value is one of the type benefiting from a fast path, we'll have to do two Marshal.load(Marshal.dump(value)) roundtrips, which for large values can be very expensive.

Capture d’écran, le 2021-04-19 à 10 37 58

By using a specialized LocalEntry type instead, we can store the Marshal payload rather than the original value. This way the overhead is reduced to a single Marshal.dump on writes and a single Marshal.load on reads.

One downside however it increased complexity, mostly because handle_expired_entry can pass a local entry directly to write_entry, so in such case we need to rebuild a regular Cache::Entry.

The local cache need to protect against mutations of both
the initial received value, and the value returned.

See:

  - rails#36656
  - rails#37587

Because of this, the overhead of the local cache end up being
quite significant. On a single read, the value will be deep duped
twice. So unless the value is one of the type benefiting from a
fast path, we'll have to do two `Marshal.load(Marshal.dump(value))`
roundtrips, which for large values can be very expensive.

By using a specialized `LocalEntry` type instead, we can store
the `Marshal` payload rather than the original value. This way
the overhead is reduced to a single `Marshal.dump` on writes
and a single `Marshal.load` on reads.
@byroot byroot merged commit 84ba451 into rails:main Apr 20, 2021
@hoangtuyb96

hoangtuyb96 commented Jun 23, 2021

Copy link
Copy Markdown

@casperisfine @byroot
I tried to benchmark the cache code in rails 6.0.4 and 6.1.0 in new project i just created.

Before benchmarking, I created a very simple dataset with 200 users. Each user only includes name, phone.
Then i stared benmarking the code.

image

In rails 6.0.4

Screen Shot 2021-06-23 at 18 10 17

In rails 6.1.0

Screen Shot 2021-06-23 at 18 32 40

I see a serious speed degradation.

So can you backport this PR into rails 6.1.3.3 or 6.1.4?

Sorry, i can't benchmark the code if writing procedure code.

@casperisfine

Copy link
Copy Markdown
Contributor Author

@hoangtuyb96 please provide a proper benchmark script using bundler/inline and benchmark-ips, so we can better understand and investigate what your issue is. A single Benchmark.measure is not a proper way to benchmark.

Also I don't understand what you are expecting me to backport, this is a performance optimization, stable branches only receive bug fixes.

@casperisfine

Copy link
Copy Markdown
Contributor Author

Here's a benchmark script example: https://gist.githubusercontent.com/casperisfine/0ccd24dc209665c46e83bcc2920dd7dc/raw/76a18809873d188501b2583a7e4636f2e83c53fe/async.rb

@casperisfine casperisfine deleted the local-cache-rafactor-take-2 branch June 23, 2021 10:18
@hoangtuyb96

Copy link
Copy Markdown

Thank you for the advice.

@kamipo

kamipo commented Jun 24, 2021

Copy link
Copy Markdown
Member

Note that we sometimes backport a patch which fixes a serious performance regression. e.g #31827

I think the point here is whether the performance regression brought about by #36656 is serious or not.

@hoangtuyb96

Copy link
Copy Markdown

I have just created issue. Please check it.
#42611

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants