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

"Moving" a subtree to a new document... #32

Closed
psikore opened this issue Jun 12, 2012 · 10 comments
Closed

"Moving" a subtree to a new document... #32

psikore opened this issue Jun 12, 2012 · 10 comments
Assignees

Comments

@psikore
Copy link

psikore commented Jun 12, 2012

I am trying to convert a large codebase from using tinyxml to tinyxml2.

In the original code it moves a subtree from one document to another via this way:

Parent->LinkRemoveChild(Element);
NewDoc->LinkEndChild(Element);

but with tinyxml2 it 'controls' the memory model so LinkRemoveChild() no longer exists.

Do you have any suggestions on how to 'move/transfer' a subtree of nodes from one document to another efficiently? ie. Least amount of code futz and smallest amount of memory allocation?

Do I need to use the Visitor pattern now somehow?

@leethomason
Copy link
Owner

This is the drawback of the memory managed approach, definitely.

Check out "ShallowClone". It copies a node, but not the nodes children, and is the main tool for the job. (There is example code in xmltest.cpp). For copying a full document, you'll need a recursive walk, and visit() seems like a reasonable choice although i haven't tried it.

Implementing "DeepClone" in the API is the right way, but I haven't implemented it yet.

@psikore
Copy link
Author

psikore commented Jun 14, 2012

From my investigation it seems like the major memory problem in Tiny1 arises from the constant string reserve calls as xml is parsed into TiXmlString. It seems like pooling the strings in a string memory allocator would cleanup the allocations and still let the nodes be dynamically allocated? As it is I'm finding it tough to work out how to use Tiny2 when the code I am porting "picks" sub-trees out of an existing doc to create a new doc.

@leethomason
Copy link
Owner

Yeah, it's involved. The strings should be put in at least a per-document string pool. That's not required for a recursive copy, but certainly is in the spirit of things. But we could start with a recursive copy first and then go from there.

@psikore
Copy link
Author

psikore commented Jun 20, 2012

For what its worth I modified TinyXML1 locally to support allocator 'hooks' such as:
TIXML_EXTERNFUNC( void , AllocHook )( size_t in_size );
then modified the tinystr.h function to call these hooks instead of directly allocating int arrays - ie...
size_t bytesToAllocate = intsNeeded * sizeof(int);
rep
= reinterpret_cast<Rep_>(TinyXML::AllocHook(bytesToAllocate));
instead of:
rep_ = reinterpret_cast<Rep*>( new int[ intsNeeded ] );
then I fed these allocations into a very fast fixed storage allocator.

In my tests this speeds up XML parsing by 3.5 times (I loaded 40 odd big xml files previously in 50seconds and now in 18 seconds).

If the lib can support the user settting up an external allocator as per above that will be helpful to others I bet... although its easy to modify since all the code is there. ;).

@leethomason
Copy link
Owner

Yeah, the main performance gain with TinyXML-2 is the memory management. Putting a better system into TinyXML-1 I'm sure helps there as well. But TinyXML-2 doesn't use statics or globals, which also helps with the threading model. I wanted to be sure that multiple XMLDocuments could be used simultaneously on multiple threads without locks. But to be clear: a given TinyXML-2 XMLDocument can only be used on a single thread.

@cyrilchampier
Copy link

The deepCloning function would be greatly appreciated !
Does anyone have already a working visitor sample to do the same job ?

@petko
Copy link

petko commented May 26, 2015

+1 for DeepClone!

@gbjbaanb
Copy link

+another one for DeepClone, this functionality shouldn't have been lost from the upgrade.

@leethomason
Copy link
Owner

Please see PR #558 (awaiting feedback until June 4th 2017)

@leethomason leethomason self-assigned this Jun 2, 2017
@leethomason
Copy link
Owner

PR #558 merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants