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

Memory management: recursively reference each parent xmlNode #360

Closed
wants to merge 6 commits into from

Conversation

rchipka
Copy link
Member

@rchipka rchipka commented Nov 15, 2015

Changes made by this pull request:

  • struct xmlNode now has a refs property
  • XmlNodes no longer use ObjectWrap::Ref or ObjectWrap::Unref
  • Reference counts are stored in struct xmlNode
  • XmlElement functions that require refcount logic have been moved to XmlNode
  • Various XML modification functions such as remove and add_child now have refcount logic
  • libxmljs.nodeCount() returns the number of nodes still in memory
  • Added test to make sure there are no remaining nodes after all tests have completed

@teleological
Copy link
Collaborator

I will continue to review tomorrow. One direction that needs further study is interplay with libxml functions that mutate the graph.

@rchipka
Copy link
Member Author

rchipka commented Nov 16, 2015

@teleological Thanks for the in-depth review

@teleological
Copy link
Collaborator

It looks like however we resolve some issues – especially those related to addChild – we will break backwards compatibility, so a major version bump is probably in the works. But before we move to a new version, I think we should, as far as possible, fix the issue on 0.* without forcing an upgrade to 1.*. Which is to say, I think we should, as far as possible, separate backwards-compatible changes from those which aren't.

So for example, import_element should not eagerly copy in 0.* – but I don't think it really needs to do so, since the effect on addChild of not copying with the new refcount system is not worse than the status quo: The risk of leaving zombie nodes in the tree when freeing wrappers is the same. Or am I missing something?

Perhaps the copying is more targeted at the add sibling functions, which unlink the added node from any prior parent. By copying, it's not necessary to adjust the refcount at the origin node – at the expense of backwards compatibility, since it remains attached. It seems to me that it would be possible to account for the unlinking behavior without defeating it.

}

// recursively unref each parent
void XmlNode::unref(int count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little worried about this approach. Rather than explicitly counting references, why not hold an object which tracks referencing, similar to a smart pointer. This would be much safer than explicitly counting

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

3 participants