Menu
▾
▴
#356 Optimize implementation of XML_ParserReset
The current implementation of XML_ParserReset sets the
user data pointer and the handlers to NULL. So, the
user has to set these again. At a first glance, there
is no need to do things this way. Instead,
XML_ParserReset could reset the internal parser data
only and leave user data and handlers untouched. So
after this function has been called, the parser would
be ready to start parsing a new document (just like
stated in the documentation of this function). To
completely reset the parser, the user would still be
able to call XML_ParserFree and XML_ParserCreate.
Stefan Letz.
stefan.letz <at> de.ibm.com
Logged In: YES
user_id=290026
I have created a patch against current CVS (xmlparse.c).
XML_ParseReset will not clear any of the settings
caused by these API functions:
- all call-back handler setters (XML_Set...Handler)
- XML_SetUserData
- XML_SetBase
- XML_UseParserAshandlerArg
- XML_SetExternalEntityRefHandlerArg
Please try the patch and report any problems it may cause.
XML_ParserReset patch
Logged In: YES
user_id=3066
While I don't expect this change to affect most
applications, it may well affect several in unexpected
ways that are difficult to debug.
For applications which set the handlers and never change
them, it's not a problem, and does save some overhead when
initializing the reset parser.
A different approach to using the handlers, however, is to
change the handlers as information is loaded. This is
certainly done in at least one language binding (Python's
PyExpat) to deal with error conditions; it's also a
capability exposed to the user (which I know I've used).
I think a new name should be assigned for the new
semantics; there's no reason we can't support both.
Logged In: YES
user_id=290026
So, you suggest a new API member, let's call it
XML_ParserPartialReset(parser, ...), which would
then keep the handlers and certain other settings.
I guess this is for backward compatibility, otherwise
I would have suggested adding another argument to the
call, like "XML_BOOL clearAll", as i don't like adding too
many API members.
Logged In: YES
user_id=3066
Perhaps XML_ParserResetDocument()? "Partial" isn't clear
as to the intent.
I understand the concern about too many API members. I
think a good rule of thumb is that if you have a simple
boolean that will (in practice) always be passed a
constant value, it makes more sense to use separate entry
points. If we expected someone to compute the value (or
load from preferences, or whatever), it would make more
sense to support it as an argument.
The backward compatibility issue would still require a
separate entry point in this case, however.
Logged In: YES
user_id=290026
Maybe its the new member that could gain an extra argument.
This could be an enum, so that we could add more choices
later (if ever needed).
XML_ParserResetDocument() isn't that clear to me either.
What about XML_ParserResetCustom()? Or some name that
shows that there are choices?
Logged In: YES
user_id=3066
The new function could be XML_ParserResetAspects(parser,
aspects), where aspects is a mask of bits defined in the
enum. The initial set of aspects could include
XML_ASPECT_USERDATA
XML_ASPECT_HANDLERS
XML_ASPECT_DOCUMENT_CONTEXT
I think that covers the aspects that are currently dealt
with, but haven't reviewed the current code or the patch.
Logged In: YES
user_id=290026
Your last suggestion seems reasonable.
What about this - let's wait until after Expat 2.0, as the
subsequent releases won't be backwards compatible anyway.
Then we can simply add the "aspects" parameter to the
existing XML_ParserReset() function without having to worry
about compatibility.
Logged In: YES
user_id=3066
Either adding it now or after works for me, but after
probably makes more sense given that we're trying to wrap
up a stable 2.0.
My own suspicion is that the cost of re-initializing a
parser should be thought of as two separate parts:
re-initializing the parse state within Expat, and
re-initializing the user-provided information (callbacks
and user-data). The parse state involves a whole pile of
memory structures that the user has no direct access to
(or control over the cost of re-initialization), and the
user-provided information is most a collection of pointers
for which the initialization cost is dominated by the
function call overhead.
This leads me to think that there's no substantial
performance benefit in this for most applications, but
there may be substantial benefit for apps running on
embedded processors.
It would be interesting to know if Sebastian's application
is running in an embedded environment, and if not, what
the motivation for the original question might be. If the
motivation is not efficiency but convenience, then it can
certainly wait for Expat 3.
FTR, the original post that generated this tracker issue
can be found in the expat-discuss mailing list archives:
http://mail.libexpat.org/pipermail/expat-discuss/2005-January/001726.html
Logged In: YES
user_id=290026
I totally agree with your conceptual separation between
re-initializing internal state and clearing user settings.
Changing how the latter behaves is mostly a matter of
convenience, I agree as well.
IMO, the bulk of applications would benefit from having
user settings *not* cleared, but there are others with
the opposite requirements.
So, I vote for modifying XML_ParserReset() for the first
release after 2.0. I also suggest that we define the "aspects"
parameter type as a set of bit flags, so that the user can
easily combine various options. E.g. (one of several possible
mappings):
XML_ASPECT_NONE = 0;
XML_ASPECT_USERDATA = 1;
XML_ASPECT_CONTENTHANDLERS = 2;
XML_ASPECT_OTHERHANDLERS = 4;
XML_ASPECT_ANYTHINGELSE = 8;
etc.
Logged In: YES
user_id=3066
I'd drop the "anything else" bit, and add XML_ASPECT_ALL =
MAX_UINT or something like that. The problem with "anything
else" is that it's semantics change (or we need a new
"anything else" value) as soon as an additional aspect is
introducted. Each bit should correspond to a well-defined
bit of state, and that mapping should never change.
If some aspect represented by a bit needs to be broken out
into separate aspects in the future, the new aspects should
receive their own bit, and the original bit should continue
to deal with all parts of that aspect.
There should also be a define that contains all the known
aspects, but nothing else. That can be used to determine if
any aspects are indicated which are not known; this should
probably be treated as an error by the library, and get
reported without making any changes.
Sounds like we're agreed on making this an Expat 3 change.
Logged In: YES
user_id=290026
You are correct about anything else - the example was just
for illustration purposes. I think we agree on all aspects here.
Logged In: YES
user_id=290026
Just a note, so that I don't forget:
XML_ParserReset will currently also clear the paramEntityParsing
member. This is also one of the settings one may not want
to be reset.
Logged In: YES
user_id=290026
Originator: NO
Changed resolution to "Postponed" for Expat 3 (if there ever is one).