Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Fixes #11512] improves cache size calculation in MemoryStore #11546

Merged
merged 1 commit into from Jul 22, 2013

Conversation

Projects
None yet
4 participants
Contributor

ssimeonov commented Jul 22, 2013

Improves ActiveSupport::Cache::MemoryStore cache size calculation.
The size of a key/entry pair is calculated via #cached_size:

def cached_size(key, entry)
  key.to_s.bytesize + entry.size + PER_ENTRY_OVERHEAD
end

The value of PER_ENTRY_OVERHEAD is 240 bytes based on an empirical
estimation
for 64-bit MRI on 1.9.3 and 2.0.

Fixes GH#11512 #11512

Contributor

ssimeonov commented Jul 22, 2013

@drogus New pull request with a single, clean commit.

Member

senny commented Jul 22, 2013

GitHub allows you to force push onto a branch to automatically update the associated pull request. This way you don't need to open new PR's.—
Sent from Mailbox for iPhone

On Mon, Jul 22, 2013 at 6:25 AM, Simeon Simeonov notifications@github.com
wrote:

@drogus New pull request with a single, clean commit.

Reply to this email directly or view it on GitHub:
#11546 (comment)

Contributor

ssimeonov commented Jul 22, 2013

@senny I did force push and have a single commit (with a different SHA1 digest than the original one) in the forked repo. After that, when I looked at the pull request I had opened, GitHub showed two commits as opposed to the single one. That was the reason I created a new PR. However, now, GH shows a single commit on the original PR. This makes the two PRs identical. Since one is closed, I don't see a problem.

Member

senny commented Jul 22, 2013

@ssimeonov We sometimes have people who open new PR's after every review so I try to explain that it's not necessary. Everything is alright though.

Member

drogus commented Jul 22, 2013

@ssimeonov sorry for nitpicking, but in the commit message you explained what you did, which can be seen in the code, what would be ideal is explanation on why you did it. Could you add a few sentences about the problem which the old way causes? Ie. when size is based only on entry.size we miss the size of the key and the size of ruby structures, which hold the entry.

[Fixes #11512] improves cache size calculation in ActiveSupport::Cach…
…e::MemoryStore

Previously, the cache size of `ActiveSupport::Cache::MemoryStore` was calculated
as the sum of the size of its entries, ignoring the size of keys and any data
structure overhead. This could lead to the calculated cache size sometimes being
10-100x smaller than the memory used, e.g., in the case of small values.

The size of a key/entry pair is now calculated via `#cached_size`:

    def cached_size(key, entry)
      key.to_s.bytesize + entry.size + PER_ENTRY_OVERHEAD
    end

The value of `PER_ENTRY_OVERHEAD` is 240 bytes based on an [empirical
estimation](https://gist.github.com/ssimeonov/6047200) for 64-bit MRI on
1.9.3 and 2.0.

Fixes GH#11512 #11512
Contributor

ssimeonov commented Jul 22, 2013

@drogus my bias is towards not duplicating existing information, hence the links to the empirical test and the original issue, which contains the detailed discussion, etc. However, I understand where you are coming from and have updated the commit message.

Member

drogus commented Jul 22, 2013

@ssimeonov it's duplication indeed, but if we start leaving the info as links to issues on each commit, you may end up opening a lot of pages to see what's going on in history. rails git history is not great, but every good commit message moves us closer to fixing this. Thanks a bunch for updating a commit message!

As a side note, it would be nice to rewrite the rake to rbx/jruby, but even using MRI based size is better than current situation so I'm going to merge it right away.

drogus added a commit that referenced this pull request Jul 22, 2013

Merge pull request #11546 from swoop-inc/ss_memory_store_cache_size
[Fixes #11512] improves cache size calculation in MemoryStore

@drogus drogus merged commit 782d2f6 into rails:master Jul 22, 2013

@rafaelfranca rafaelfranca commented on the diff Jul 22, 2013

activesupport/CHANGELOG.md
@@ -143,4 +143,17 @@
*Daniel Schierbeck*
+* Improve `ActiveSupport::Cache::MemoryStore` cache size calculation.
+ The memory used by a key/entry pair is calculated via `#cached_size`:
+
+ def cached_size(key, entry)
@rafaelfranca

rafaelfranca Jul 22, 2013

Owner

New CHANGELOG entries always on top

@drogus

drogus Jul 22, 2013

Member

We need to add this info to contributing guide, the info there seems outdated, I can take care of this.

@drogus

drogus Jul 22, 2013

Member

@rafaelfranca added at 4e211d3, thanks for heads up!

Owner

rafaelfranca commented Jul 22, 2013

This need to be backported to 4-0-stable

Member

drogus commented Jul 22, 2013

@rafaelfranca yup, 👍

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