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

Optimization for layout_init() #6547

Merged
merged 6 commits into from
Aug 28, 2019
Merged

Conversation

haberman
Copy link
Member

@haberman haberman commented Aug 22, 2019

This avoids most of the work of layout_init() by creating a template object we can memcpy() from when initializing a new object.

There is a bit of complication since we must eagerly allocate maps and repeated fields. So we have to dup these still in layout_init().

In the benchmark on my machine this takes us from 8 seconds to 2.3 seconds, or a 70% reduction in runtime overall.

@fables-tales
Copy link
Contributor

Initially this looks good for ads protos, just gathering some data on our large data set now!

@haberman
Copy link
Member Author

I'm getting the following crash on Mac Ruby 2.3. I've reproduced this locally:

-- C level backtrace information -------------------------------------------
0   ruby                                0x000000010bd7cb4d rb_vm_bugreport + 381
1   ruby                                0x000000010bc049ef rb_bug_context + 447
2   ruby                                0x000000010bce3b74 sigsegv + 68
3   libsystem_platform.dylib            0x00007fff5ffc9b5d _sigtramp + 29
4   ruby                                0x000000010bc30d71 gc_mark_ptr + 81
5   protobuf_c.bundle                   0x000000010c0759bc layout_mark + 124
6   ruby                                0x000000010bc2f9d8 gc_marks_rest + 88
7   ruby                                0x000000010bc2e160 gc_start + 2880
8   ruby                                0x000000010bc316f8 garbage_collect_with_gvl + 216
9   ruby                                0x000000010bc2959d objspace_xmalloc + 141
10  ruby                                0x000000010bd33609 generic_ivar_update + 169
11  ruby                                0x000000010bcede3c st_update + 700
12  ruby                                0x000000010bd2e01c rb_ivar_set + 460
13  protobuf_c.bundle                   0x000000010c076580 Message_alloc + 144
14  ruby                                0x000000010bc6a195 rb_obj_alloc + 117
15  ruby                                0x000000010bc6b118 rb_class_new_instance + 24
16  ruby                                0x000000010bd72832 vm_call_cfunc + 290
17  ruby                                0x000000010bd5c15f vm_exec_core + 10655

The fact that this is in layout_mark() makes me worry that the crash might already be on master.

@TeBoring TeBoring added this to the v3.10.0 milestone Aug 28, 2019
@haberman
Copy link
Member Author

I did some testing and I have been unable to reproduce this on master, but it reproduces relatively easily on this branch. So it appears the bug is in this PR after all (which is good -- I can fix it before merging).

@haberman
Copy link
Member Author

The crash that I was seeing earlier has been fixed in the latest commit. This is ready for review.

@haberman haberman merged commit d2d49bf into protocolbuffers:master Aug 28, 2019
@haberman haberman deleted the layout_clear branch December 6, 2020 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants