Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ObjectSpace.memsize_of #5032

Merged
merged 12 commits into from Jul 16, 2020
Merged

Add ObjectSpace.memsize_of #5032

merged 12 commits into from Jul 16, 2020

Conversation

RoryO
Copy link
Contributor

@RoryO RoryO commented Jul 10, 2020

Hello, this is my first significant contribution. I hope the review is not a burden.

This implements the mri method of ObjectSpace.memsize_of. With mruby we care a great deal about tracking resource use therefore I feel this is useful. All of the code is new, none copied. The mruby structures differ from C Ruby structures so existing C Ruby code isn't useful.

One difference between this implementation and the mri one is an addition of a recurse parameter. Specifying ObjectSpace.memsize_of obj, recurse: true will recursively descend into collection members and instance variables. Do let me know if I should not include this option and I will remove it.

Additionally, I added a protocol for MRB_TT_DATA objects. If a data object implements the memsize method this code will call that method and use it's return value for the size total. This allows a chance for including data objects into memory calculations, a gap I noticed from mri.

assert_equal ObjectSpace.memsize_of(false), int_size

float_size = if Object.const_defined? :Float
ObjectSpace.memsize_of 1.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of where it is useful exposing internal details. The memory size of a float is 0 with default no boxing, but is the size of an mrb_value when using word boxing.

@RoryO RoryO marked this pull request as ready for review July 15, 2020 06:52
include/mruby/hash.h Outdated Show resolved Hide resolved
@matz matz merged commit b5bf951 into mruby:master Jul 16, 2020
@matz
Copy link
Member

matz commented Jul 17, 2020

I missed that ObjectSpace.memsize_of does not recurse. It was originally designed to analyze single object allocation.
Do we really need recurring enhancement for mruby?

@RoryO
Copy link
Contributor Author

RoryO commented Jul 17, 2020

If you wish I can remove it.

@RoryO RoryO deleted the add-objspace-memsize-of branch July 17, 2020 02:25
@matz
Copy link
Member

matz commented Jul 17, 2020

I will do some fixes and refactoring to improve compatibility with CRuby. I am working on it.
Thank you!

@RoryO
Copy link
Contributor Author

RoryO commented Jul 17, 2020

Please show me if there's anything I can do to help, including working on C Ruby as well. I do not wish to burden!

@matz
Copy link
Member

matz commented Jul 17, 2020

Never mind. It's fun!

matz added a commit that referenced this pull request Jul 17, 2020
matz added a commit that referenced this pull request Jul 17, 2020
matz added a commit that referenced this pull request Jul 17, 2020
matz added a commit that referenced this pull request Jul 17, 2020
matz added a commit that referenced this pull request Jul 17, 2020
This is enhancement from CRuby's `memsize_of`. We need to change the
CRuby first for the enhancement.
matz added a commit that referenced this pull request Jul 17, 2020
matz added a commit that referenced this pull request Jul 17, 2020
Memory size of a Fiber is calculated by stack size only in CRuby.
matz added a commit that referenced this pull request Jul 17, 2020
Also avoid `mrb_funcall` to minimize VM recursion.
matz added a commit that referenced this pull request Jul 17, 2020
@shuujii
Copy link
Contributor

shuujii commented Jul 19, 2020

I've noticed that ObjectSpace.memsize_of doesn't exist in JRuby and so on. Do we need to maintain incompatible features among Ruby implementations in the mruby repository?

ObjectSpace.memsize_of may not return an exact value, so I personally find it difficult to use. Therefore, I feel it's better to provide it as an external gem.

@matz
Copy link
Member

matz commented Jul 19, 2020

Separating to a gem. Hmm. OK.

@RoryO
Copy link
Contributor Author

RoryO commented Jul 19, 2020

ObjectSpace.memsize_of may not return an exact value

Can you provide some examples? I wanted to make it accurate along with matz' corrections. I think this is an important tool for an embedded project.

@matz
Copy link
Member

matz commented Jul 19, 2020

I think he meant the result does not count alignment gaps nor allocation overhead, which are not easy to calculate.

@matz
Copy link
Member

matz commented Jul 19, 2020

I made a separated gem for memsize_of. The diff is here:
https://gist.github.com/matz/0ece6c6fbeeb7609dd44e894f276ee7e
If I commit this change, we will lose @RoryO's name and history from the repository.
Maybe he can commit the above change as a PR.

@shuujii
Copy link
Contributor

shuujii commented Jul 20, 2020

ObjectSpace.memsize_of may not return an exact value

Can you provide some examples? I wanted to make it accurate along with matz' corrections. I think this is an important tool for an embedded project.

Here are some examples. I'm not sure what value users typically expect or appropriate for the return value of ObjectSpace.memsize_of.

  • Currently, the method returns 0 for Symbol. However, creating one non-inline symbol uses one slot in mrb_state::symtbl (which may also uses a slot in mrb_state::symhash). Also, if the symbol name is not ro_data, allocate a buffer for it.
  • Currently, the method returns RVALUE size for shared String. In the case of shared Strings, I think shared buffer size (including size of mrb_shared_string) should be accounted for in one of the objects that share the same buffer. However, I don't think it's obvious which objects should be accounted for. Information may need to be added to RString to identify the object, even if it is clear. I don't think it's desirable to increase runtime costs and so on just for the feature.
  • Currently, the method retuerns 0 for Fixnum. However, it may be using a slot in literal pool.

I made a separated gem for memsize_of.

My intention was to provide it in a different repository than mruby/mruby (e.g. RoryO/mruby-os-memsize).

I personally feel that return value definition of the method is unclear. The meaning of the value may change due to changes in internal implementation because it strongly depends on the internal implementation.

Still, the methods may have some uses depending on the use case, but I'm a little wondering if it is necessary to officially provide such a seemingly unclear feature (also, I'm a little worried that the maintenance cost seems to be high against the frequency of use).

I agree with separating gems when maintaining in mruby/mruby because the method is required require 'objspace' in CRuby.

@matz
Copy link
Member

matz commented Jul 20, 2020

Considering the fact memsize_of is that highly depends on the mruby internal, it should be maintained mainly by the core developer (i.e. me).

Comment on lines +521 to +534
mrb_int
mrb_os_memsize_of_hash_table(mrb_value obj)
{
struct htable *h = mrb_hash_ptr(obj)->ht;
mrb_int segkv_size = 0;

if(h->index) segkv_size = (sizeof(struct segkv) * h->index->capa);

return sizeof(htable) +
sizeof(segindex) +
(sizeof(segment) * h->size) +
segkv_size;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The result seems to be incorrect.

Example (64-bit no-boxing):

Hash Size Expected Result Actual Result
1 184 72
8 360 184
16 600 312
24 1176 1464
32 1800 2616
  • IV table size should also be included.

  • The result may not fit into mrb_int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this example! Do you mean that a Hash object should also include mrb_obj_iv_tbl_memsize, like this

--- a/mrbgems/mruby-os-memsize/src/memsize.c
+++ b/mrbgems/mruby-os-memsize/src/memsize.c
@@ -74,6 +74,7 @@ os_memsize_of_object(mrb_state* mrb, mrb_value obj)
     }
     case MRB_TT_HASH: {
       size += mrb_objspace_page_slot_size() +
+              mrb_obj_iv_tbl_memsize(mrb, obj) +
               mrb_os_memsize_of_hash_table(obj);
       break;
     }

Do you mean the numeric limit of mrb_int? That has a minimum limit of INT32_MAX. I suppose it's possible for one object consuming 2gb of memory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that a Hash object should also include mrb_obj_iv_tbl_memsize, like this

Since IV table is part of a Hash object, I think it's natural that its memory size be included in the result of mrb_os_memsize_of_hash_table().

However, in CRuby, for example, the result of rb_ary_memsize() seems not to include memory size of IV table, so I think that it will look like the code you presented if follow CRuby.

Do you mean the numeric limit of mrb_int?

Yes. I have no opinion on the behavior when it exceeds MRB_INT_MAX, but I'd like to know if it is not taken into consideration or if it is considered and fixed.

dearblue added a commit to dearblue/mruby that referenced this pull request Jul 21, 2020
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 21, 2020
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 21, 2020
And, in the calculation of the instance variable size, the fraction was
always rounded down because of division of integers, so fix it.
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 21, 2020
If it qualify a return type that is not a pointer with `const`, the
compiler ignores it.
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 24, 2020
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 24, 2020
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 24, 2020
And, in the calculation of the instance variable size, the fraction was
always rounded down because of division of integers, so fix it.

At the same time, test items that are no longer passed due to this
change are deleted.
dearblue added a commit to dearblue/mruby that referenced this pull request Jul 24, 2020
If it qualify a return type that is not a pointer with `const`, the
compiler ignores it.
shuujii added a commit to shuujii/mruby that referenced this pull request Nov 10, 2020
## Implementation Summary

* Change entry list from segmented list to flat array.
* Change value of hash bucket from pointer to entry to index of entry list,
  and represent it by variable length bits according to capacity of hash
  buckets.
* Store management information about entry list and hash table to `struct
  RHash` as much as possible.

## Benchmark Summary

Only the results of typical situations on 64-bit Word-boxing are present
here. For more detailed information, including consideration, see below
(although most of the body is written in Japanese).

* https://shuujii.github.io/mruby-hash-benchmark

### Memory Usage

Lower value is better.

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 |          344B |          256B |   0.74419x |
|        40 |        1,464B |          840B |   0.57377x |
|       200 |        8,056B |        3,784B |   0.46971x |
|       500 |       17,169B |        9,944B |   0.57949x |

### Performance

Higher value is better.

#### `mrb_hash_set`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 |  1.41847M i/s |  1.36004M i/s |   0.95881x |
|        40 |  0.39224M i/s |  0.31888M i/s |   0.81296x |
|       200 |  0.03780M i/s |  0.04290M i/s |   1.13494x |
|       500 |  0.01225M i/s |  0.01314M i/s |   1.07275x |

#### `mrb_hash_get`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 | 26.05920M i/s | 30.19543M i/s |   1.15872x |
|        40 | 44.26420M i/s | 32.75781M i/s |   0.74005x |
|       200 | 44.55171M i/s | 31.56926M i/s |   0.70860x |
|       500 | 39.19250M i/s | 29.73806M i/s |   0.75877x |

#### `mrb_hash_each`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 | 25.11964M i/s | 30.34167M i/s |   1.20789x |
|        40 | 11.74253M i/s | 13.25539M i/s |   1.12884x |
|       200 |  2.01133M i/s |  2.97214M i/s |   1.47770x |
|       500 |  0.87411M i/s |  1.21178M i/s |   1.38631x |

#### `Hash#[]=`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 |  0.50095M i/s |  0.56490M i/s |   1.12764x |
|        40 |  0.19132M i/s |  0.18392M i/s |   0.96129x |
|       200 |  0.03624M i/s |  0.03256M i/s |   0.89860x |
|       500 |  0.01527M i/s |  0.01236M i/s |   0.80935x |
#### `Hash#[]`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 | 11.53211M i/s | 12.78806M i/s |   1.10891x |
|        40 | 15.26920M i/s | 13.37529M i/s |   0.87596x |
|       200 | 15.28550M i/s | 13.36410M i/s |   0.87430x |
|       500 | 14.57695M i/s | 12.75388M i/s |   0.87494x |

#### `Hash#each`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 |  0.30462M i/s |  0.27080M i/s |   0.88898x |
|        40 |  0.12912M i/s |  0.11704M i/s |   0.90642x |
|       200 |  0.02638M i/s |  0.02402M i/s |   0.91071x |
|       500 |  0.01066M i/s |  0.00959M i/s |   0.89953x |

#### `Hash#delete`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 |  7.84167M i/s |  6.96419M i/s |   0.88810x |
|        40 |  6.91292M i/s |  7.41427M i/s |   1.07252x |
|       200 |  3.75952M i/s |  7.32080M i/s |   1.94727x |
|       500 |  2.10754M i/s |  7.05963M i/s |   3.34970x |

#### `Hash#shift`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 | 14.66444M i/s | 13.18876M i/s |   0.89937x |
|        40 | 11.95124M i/s | 11.10420M i/s |   0.92913x |
|       200 |  5.53681M i/s |  7.88155M i/s |   1.42348x |
|       500 |  2.96728M i/s |  5.40405M i/s |   1.82121x |

#### `Hash#dup`

| Hash Size |   Baseline    |      New      |   Factor   |
|----------:|--------------:|--------------:|-----------:|
|        16 |  0.15063M i/s |  5.37889M i/s |  35.71024x |
|        40 |  0.06515M i/s |  3.38196M i/s |  51.91279x |
|       200 |  0.01359M i/s |  1.46538M i/s | 107.84056x |
|       500 |  0.00559M i/s |  0.75411M i/s | 134.88057x |

### Binary Size

Lower value is better.

|    File    |   Baseline    |      New      |  Factor   |
|:-----------|--------------:|--------------:|----------:|
| mruby      |      730,408B |      734,176B |  1.00519x |
| libmruby.a |    1,068,134B |    1,072,846B |  1.00441x |

## Other Fixes

The following issues have also been fixed in the parts where there was some
change this time.

* [Heap use-after-free in `Hash#value?`](https://gist.github.com/shuujii/30e4fcd5844a4112a0ecd4a5b3483101#file-heap-use-after-free-in-hash-value-md)
* [Heap use-after-free in `ht_hash_equal`](https://gist.github.com/shuujii/30e4fcd5844a4112a0ecd4a5b3483101#file-heap-use-after-free-in-ht_hash_equal-md)
* [Heap use-after-free in `ht_hash_func`](https://gist.github.com/shuujii/30e4fcd5844a4112a0ecd4a5b3483101#file-heap-use-after-free-in-ht_hash_func-md)
* [Heap use-after-free in `mrb_hash_merge`](https://gist.github.com/shuujii/30e4fcd5844a4112a0ecd4a5b3483101#file-heap-use-after-free-in-mrb_hash_merge-md)
* [Self-replacement does not work for `Hash#replace`](https://gist.github.com/shuujii/30e4fcd5844a4112a0ecd4a5b3483101#file-self-replacement-does-not-work-for-hash-replace-md)
* [Repeated deletes and inserts increase memory usage of `Hash`](https://gist.github.com/shuujii/30e4fcd5844a4112a0ecd4a5b3483101#file-repeated-deletes-and-inserts-increase-memory-usage-of-hash-md)
* [`Hash#rehash` does not reindex completely](https://gist.github.com/shuujii/30e4fcd5844a4112a0ecd4a5b3483101#file-hash-rehash-does-not-reindex-completely-md)
* `mrb_hash_delete_key` does not cause an error for frozen object
* `mrb_hash_new_capa` does not allocate required space first
* [`mrb_os_memsize_of_hash_table` result is incorrect](mruby#5032 (comment))
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.

None yet

4 participants