-
-
Notifications
You must be signed in to change notification settings - Fork 896
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
Fix compaction on nodes #2579
Fix compaction on nodes #2579
Conversation
Note: this should probably be backported to v1.13.x in a patch release. I'll do that in a separate PR once this is on |
Sorry for being slow to merge this. I will work on it (and the backport?) this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. You should even be able to get rid of the casts, but they're not doing any harm.
Edit: I meant the approval to mean the updated callback signatures looked good. I did notice one of the tests shows a memory leak reported by Valgrind.
Unconditionally set a mark function on the node wrapper. We will later refactor this to use TypedData_Wrap_Struct and we don't want to conditionally set mark functions on that Co-Authored-By: Mike Dalessio <mike.dalessio@gmail.com>
0fd5321
to
21e759a
Compare
I rebased and made some minor changes. |
21e759a
to
c84a345
Compare
I've added a commit 1c74ba1, which I would like to squash, which uses a macro This results in a smaller diff, since The resulting combined diff is here: https://github.com/sparklemotion/nokogiri/compare/b370fbd290343e93f20e6bf12522c5ba3f2f5caf..1c74ba18fccc4e809d62c62f2f8ee536aae3ee94 @tenderlove @eightbitraptor would love a quick review when you have a moment. |
Added one more commit onto the end to remove |
@tenderlove I've got one last question for you, which is "why are we using I think I know the answer (but would like confirmation): the alternative of using Does that sound about right? |
The memcheck failure is because |
380dfda
to
7a8ccd6
Compare
Yes, about. The problem I found is that we have so many objects that inherit from I'd prefer to use the "typed" unwrappers everywhere, but I couldn't figure out a "proper" way to do that with C objects that use inheritance (without adding tons of checks). |
@tenderlove Thanks. This is going to go green so I'm going to clean up commit history (squash some of my changes) and let it go through CI one more time. |
This commit introduces a `Noko_Node_Get_Struct` macro for unwrapping the underlying data pointer from Nokogiri objects. Then we change all places that unwrap nodes to use the new macro. We also converted xmlNode to use `TypedData_Wrap_Struct` so we can provide a compaction function. Note that we use DATA_PTR instead of "typed" unwrappers everywhere because the large number of C structs that inherit from xmlNode would make the unwrapping code unwieldy and type-check-heavy. Co-Authored-By: Matt Valentine-House <matt@eightbitraptor.com> Co-Authored-By: Mike Dalessio <mike.dalessio@gmail.com>
This fixes the case where nodes moves and we need to update the private references. Co-Authored-By: Matt Valentine-House <matt@eightbitraptor.com> Co-Authored-By: Mike Dalessio <mike.dalessio@gmail.com>
Co-Authored-By: Mike Dalessio <mike.dalessio@gmail.com>
We don't have `rb_gc_location` everywhere, so check for it in the extconf and then conditionally set the compaction callback Co-Authored-By: Mike Dalessio <mike.dalessio@gmail.com>
7a8ccd6
to
c59dfaf
Compare
for warnings triggered by the compaction test. - for memory allocated in ary_heap_realloc - for memory added to the mark stack in stack_chunk_alloc
c59dfaf
to
a036080
Compare
which changed the typed-data semantics around xmlNode in order to fully support compaction see sparklemotion/nokogiri#2579
This PR is an attempt to fix compaction issues from #2578. We've split the PR in to 3 commits.
The first commit consistently sets a mark function on the xml node. We need this because we need to provide a static struct to
TypedData_Wrap_Struct
.The second commit introduces a helper function for unwrapping xml nodes. Unfortunately, xml nodes use a union type, and subclasses want to unwrap the type in to different things, so we had to extract a function that would unwrap the nodes correctly.
The third commit actually fixes compaction by adding a callback for updating references.
Fixes #2578