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

Make compaction available for Oj::Doc #653

Merged
merged 2 commits into from Apr 9, 2021

Conversation

BuonOmo
Copy link
Contributor

@BuonOmo BuonOmo commented Apr 9, 2021

Introduce compaction to fast.c, only if rb_gc_mark_movable is
available. Otherwise, keep a version that supports ruby <2.7.

Since rb_data_object_wrap and TypedData_Wrap_Struct is available
in every supported versions (eg >=2.4.10), I've also removed support
for earlier versions.

Result for compaction is great. I've tested it by allocating and
removing arbitrary sized json. For a count of 70 pages, we can see
that we used to need 68 pages after compaction,and now 61 pages. Of
course this test include also lots of noise, but if we count pinned
objects (objects that are marked in C extension as uncollectible),
we can see that for a benchmark with no allocations, we have the
same amount as a benchmark with this commit with thousands of allocations
(499 pinned objects total), and before this, Oj::Doc used to generate
5k more objects (5499 pinned objects total).

Hence this is a win.

Here's the result of my tests after a GC.compact (reddots are pinned objects):

before patch after patch
5499 pinned objects 499 pinned objects
before patch after

see #650 for scripts that generated these results.

Signed-off-by: Ulysse Buonomo buonomo.ulysse@gmail.com

Introduce compaction to `fast.c`, only if `rb_gc_mark_movable` is
available. Otherwise, keep a version that supports ruby <2.7.

Since `rb_data_object_wrap` and `TypedData_Wrap_Struct` is available
in every supported versions (eg >=2.4.10), I've also removed support
for earlier versions.

Result for compaction is great. I've tested it by allocating and
removing arbitrary sized json. For a count of 70 pages, we can see
that we used to need 68 pages after compaction,and now 61 pages. Of
course this test include also lots of noise, but if we count pinned
objects (objects that are marked in C extension as uncollectible),
we can see that for a benchmark with no allocations, we have the
same amount as a benchmark with this commit with thousands of allocations
(499 pinned objects total), and before this, `Oj::Doc` used to generate
5k more objects (5499 pinned objects total).

Hence this is a win, see ohler55#650 for a graphical result.

Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
@@ -328,7 +328,7 @@ def test_pipe
IO.pipe do |read_io, write_io|
if fork
write_io.close
Oj.sc_parse(handler, read_io) {|v| p v}
Oj.sc_parse(handler, read_io)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ohler55 this one was outputing to tests, I looks like this was unwanted. Not 100% sure though

According to clang's documentation:

> When using `-style=file`, **clang-format** for each input file will
> try to find the `.clang-format` file located in the closest parent
> directory of the input file. When the standard input is used, the
> search is started from the current directory.

Hence the `.clang-format` seems more appropriate

Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>
@BuonOmo
Copy link
Contributor Author

BuonOmo commented Apr 9, 2021

I've also renamed the clang-format file according to documentation 🙂

@ohler55
Copy link
Owner

ohler55 commented Apr 9, 2021

Thanks for the additions and flexibility in getting it in.

@ohler55 ohler55 merged commit b12cbfb into ohler55:develop Apr 9, 2021
@BuonOmo BuonOmo deleted the gc-compaction-compatibility branch April 9, 2021 13:58
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

2 participants