You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by John Austin <jw...@sympatico.ca> on 2003/12/19 14:13:45 UTC
Is this a coding flaw ?
I found the following snippet in the class FOFileHandler:
===============================================================
/**
* @see org.apache.fop.apps.InputHandler#getParser()
*/
public XMLReader getParser() throws FOPException {
return createParser();
}
===============================================================
and the createParser() method
===============================================================
/**
* Creates <code>XMLReader</code> object using default
* <code>SAXParserFactory</code>
* @return the created <code>XMLReader</code>
* @throws FOPException if the parser couldn't be created or
configured for proper operation.
*/
protected static XMLReader createParser() throws FOPException {
try {
SAXParserFactory factory = SAXParserFactory.newInstance();
factory.setNamespaceAware(true);
factory.setFeature(
"http://xml.org/sax/features/namespace-prefixes", true);
return factory.newSAXParser().getXMLReader();
<snip/>
===============================================================
Now it would seem to me that a 'getter' method should not go around
creating objects every time it needs to. It hust doesn't look right.
I assume that SAXParserFactory is thread-safe.
--
John Austin <jw...@sympatico.ca>
Re: Is this a coding flaw ?
Posted by Ben Galbraith <ju...@galbraiths.org>.
John Austin wrote:
> Nothing to do with optimization. Just noticed some wrongness
> that has the possibility to be pathological wrongness. Classes
> should preclude the possibility of erroneous use. The subject
> was making a URL resolver thread-safe. The class in question is
> a source of state information needed later by the resolver.
Whoops! Was unsure about butting in the first place, certainly don't
mean to offend. But since I already have, let me keep going and explain
myself. :-)
The method is not necessarily pathological, right? If the intent of the
method is to provide both a convenience method and a point of
centralization (in case the parser is in the future overridden via one
of the steps in the SAXParserFactory resolution mechanism or a
hard-coded instance class name), it certainly has done so. It's
legitimate for an app to want to know the class name of the SAX parser
that will be produced by the app at any given moment.
Hence, since the method's existence has the potential to be legitimate
(without checking its usages its impossible to know the exact contract
since it is not documented), I assumed you were concerned with the
performance implications of working around possible thread unsafety.
So, since SAXParserFactory is *not* thread-safe, the createParser()
method ought to be synchronized and only if the synchronization presents
a measurably unacceptable latency should caching mechanisms be
considered (IMHO).
These are the thoughts that led to my earlier message, at least, feeble
though they are. ;-)
Ben
>
> [Lucky thing we didn't mention the dirty knife!]
>
> On Fri, 2003-12-19 at 11:50, Ben Galbraith wrote:
>
>>Jeremias Maerki wrote:
>>
>>>Hmm, again, we could probably cache the value. Not very elegant, of
>>>course, but how else do we get that value which is used in several
>>>places?
>>
>>Just an outsider's point-of-view: it probably doesn't make sense to
>>waste time optimizing code like this unless a profiler indicates that
>>it's a bottleneck.
>>
>>Randomly searching through code for potential inefficiencies has widely
>>been disproven as an effective optimization technique. ;-)
>>
>>Ben
>>
>>
>>>On 19.12.2003 13:57:26 John Austin wrote:
>>>
>>>
>>>>And of course, I missed the fact that the last method in the class
>>>>contains a pathological use. To get the name of this class, we create a
>>>>parser ?
>>>>
>>>> /**
>>>> * Returns the fully qualified classname of the standard XML parser
>>>>for FOP
>>>> * to use.
>>>> * @return the XML parser classname
>>>> */
>>>> public static final String getParserClassName() {
>>>> try {
>>>> return createParser().getClass().getName();
>>>> } catch (FOPException e) {
>>>> return null;
>>>> }
>>>> }
>>>
>>>
>>>
>>>Jeremias Maerki
>>>
Re: Is this a coding flaw ?
Posted by John Austin <jw...@sympatico.ca>.
Nothing to do with optimization. Just noticed some wrongness
that has the possibility to be pathological wrongness. Classes
should preclude the possibility of erroneous use. The subject
was making a URL resolver thread-safe. The class in question is
a source of state information needed later by the resolver.
[Lucky thing we didn't mention the dirty knife!]
On Fri, 2003-12-19 at 11:50, Ben Galbraith wrote:
> Jeremias Maerki wrote:
> > Hmm, again, we could probably cache the value. Not very elegant, of
> > course, but how else do we get that value which is used in several
> > places?
>
> Just an outsider's point-of-view: it probably doesn't make sense to
> waste time optimizing code like this unless a profiler indicates that
> it's a bottleneck.
>
> Randomly searching through code for potential inefficiencies has widely
> been disproven as an effective optimization technique. ;-)
>
> Ben
>
> >
> > On 19.12.2003 13:57:26 John Austin wrote:
> >
> >>And of course, I missed the fact that the last method in the class
> >>contains a pathological use. To get the name of this class, we create a
> >>parser ?
> >>
> >> /**
> >> * Returns the fully qualified classname of the standard XML parser
> >>for FOP
> >> * to use.
> >> * @return the XML parser classname
> >> */
> >> public static final String getParserClassName() {
> >> try {
> >> return createParser().getClass().getName();
> >> } catch (FOPException e) {
> >> return null;
> >> }
> >> }
> >
> >
> >
> > Jeremias Maerki
> >
--
John Austin <jw...@sympatico.ca>
Re: Is this a coding flaw ?
Posted by Ben Galbraith <ju...@galbraiths.org>.
Jeremias Maerki wrote:
> Hmm, again, we could probably cache the value. Not very elegant, of
> course, but how else do we get that value which is used in several
> places?
Just an outsider's point-of-view: it probably doesn't make sense to
waste time optimizing code like this unless a profiler indicates that
it's a bottleneck.
Randomly searching through code for potential inefficiencies has widely
been disproven as an effective optimization technique. ;-)
Ben
>
> On 19.12.2003 13:57:26 John Austin wrote:
>
>>And of course, I missed the fact that the last method in the class
>>contains a pathological use. To get the name of this class, we create a
>>parser ?
>>
>> /**
>> * Returns the fully qualified classname of the standard XML parser
>>for FOP
>> * to use.
>> * @return the XML parser classname
>> */
>> public static final String getParserClassName() {
>> try {
>> return createParser().getClass().getName();
>> } catch (FOPException e) {
>> return null;
>> }
>> }
>
>
>
> Jeremias Maerki
>
Re: Is this a coding flaw ?
Posted by Jeremias Maerki <de...@greenmail.ch>.
Hmm, again, we could probably cache the value. Not very elegant, of
course, but how else do we get that value which is used in several
places?
On 19.12.2003 13:57:26 John Austin wrote:
> And of course, I missed the fact that the last method in the class
> contains a pathological use. To get the name of this class, we create a
> parser ?
>
> /**
> * Returns the fully qualified classname of the standard XML parser
> for FOP
> * to use.
> * @return the XML parser classname
> */
> public static final String getParserClassName() {
> try {
> return createParser().getClass().getName();
> } catch (FOPException e) {
> return null;
> }
> }
Jeremias Maerki
Re: Is this a coding flaw ?
Posted by John Austin <jw...@sympatico.ca>.
On Fri, 2003-12-19 at 10:02, Jeremias Maerki wrote:
> I should be thread-safe, the way it is used here. You could of course,
> cache the SAXParserFactory instance but I doubt the performance
> improvement would be measurable. getParser is probably not the best name
> if you look at it from a bean-oriented angle but it's not that it's
> called many times anyway. Do you think we should rename it?
As long as we are certain that it is being used correctly, probably
not necessary. Just jumped a bit when I saw the possibility that it
would be easily mis-used.
>
> On 19.12.2003 13:13:45 John Austin wrote:
> > I found the following snippet in the class FOFileHandler:
> >
> > ===============================================================
> > /**
> > * @see org.apache.fop.apps.InputHandler#getParser()
> > */
> > public XMLReader getParser() throws FOPException {
> > return createParser();
> > }
> > ===============================================================
> >
> > and the createParser() method
> >
> > ===============================================================
> > /**
> > * Creates <code>XMLReader</code> object using default
> > * <code>SAXParserFactory</code>
> > * @return the created <code>XMLReader</code>
> > * @throws FOPException if the parser couldn't be created or
> > configured for proper operation.
> > */
> > protected static XMLReader createParser() throws FOPException {
> > try {
> > SAXParserFactory factory = SAXParserFactory.newInstance();
> > factory.setNamespaceAware(true);
> > factory.setFeature(
> > "http://xml.org/sax/features/namespace-prefixes", true);
> > return factory.newSAXParser().getXMLReader();
> >
> > <snip/>
> >
> > ===============================================================
> >
> > Now it would seem to me that a 'getter' method should not go around
> > creating objects every time it needs to. It hust doesn't look right.
> >
> > I assume that SAXParserFactory is thread-safe.
>
>
> Jeremias Maerki
--
John Austin <jw...@sympatico.ca>
Re: Is this a coding flaw ?
Posted by Ben Galbraith <ju...@galbraiths.org>.
J.Pietschmann wrote:
> That's not the only issue with the Batik bridge - the interface
> has been grown for some time.
>
> Anybody out there willing to fix this?
What's wrong with it? Any functionality broken or is the code just obtuse?
Re: Is this a coding flaw ?
Posted by "J.Pietschmann" <j3...@yahoo.de>.
John Austin wrote:
> And of course, I missed the fact that the last method in the class
> contains a pathological use. To get the name of this class, we create a
> parser ?
That's legacy code from pre-JAXP times. IIRC it is still used
in the Batik bridge. The bridge needs an XML parser to parse
references in the SVG to referencable items in other SVGs.
The most ugly issue is mainly how to treat
gradient="url('#foo')"
stuff in embedded SVG code: #foo could in principle refer to
an unresolvable node at the time it is needed because it is in
another embedded code snippet further down the tree. FOP treats
this as an error, embedded SVGs have to be self-contained. The
spec is not helpful for guidance here.
That's not the only issue with the Batik bridge - the interface
has been grown for some time.
Anybody out there willing to fix this?
Re: Is this a coding flaw ?
Posted by John Austin <jw...@sympatico.ca>.
On Fri, 2003-12-19 at 10:02, Jeremias Maerki wrote:
> I should be thread-safe, the way it is used here. You could of course,
> cache the SAXParserFactory instance but I doubt the performance
> improvement would be measurable. getParser is probably not the best name
> if you look at it from a bean-oriented angle but it's not that it's
> called many times anyway. Do you think we should rename it?
>
> On 19.12.2003 13:13:45 John Austin wrote:
> > I found the following snippet in the class FOFileHandler:
And of course, I missed the fact that the last method in the class
contains a pathological use. To get the name of this class, we create a
parser ?
/**
* Returns the fully qualified classname of the standard XML parser
for FOP
* to use.
* @return the XML parser classname
*/
public static final String getParserClassName() {
try {
return createParser().getClass().getName();
} catch (FOPException e) {
return null;
}
}
--
John Austin <jw...@sympatico.ca>
Re: Is this a coding flaw ?
Posted by Jeremias Maerki <de...@greenmail.ch>.
I should be thread-safe, the way it is used here. You could of course,
cache the SAXParserFactory instance but I doubt the performance
improvement would be measurable. getParser is probably not the best name
if you look at it from a bean-oriented angle but it's not that it's
called many times anyway. Do you think we should rename it?
On 19.12.2003 13:13:45 John Austin wrote:
> I found the following snippet in the class FOFileHandler:
>
> ===============================================================
> /**
> * @see org.apache.fop.apps.InputHandler#getParser()
> */
> public XMLReader getParser() throws FOPException {
> return createParser();
> }
> ===============================================================
>
> and the createParser() method
>
> ===============================================================
> /**
> * Creates <code>XMLReader</code> object using default
> * <code>SAXParserFactory</code>
> * @return the created <code>XMLReader</code>
> * @throws FOPException if the parser couldn't be created or
> configured for proper operation.
> */
> protected static XMLReader createParser() throws FOPException {
> try {
> SAXParserFactory factory = SAXParserFactory.newInstance();
> factory.setNamespaceAware(true);
> factory.setFeature(
> "http://xml.org/sax/features/namespace-prefixes", true);
> return factory.newSAXParser().getXMLReader();
>
> <snip/>
>
> ===============================================================
>
> Now it would seem to me that a 'getter' method should not go around
> creating objects every time it needs to. It hust doesn't look right.
>
> I assume that SAXParserFactory is thread-safe.
Jeremias Maerki