Re: Is this a coding flaw ?
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 ?
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 ?
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 ?
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 <[EMAIL PROTECTED]>
Re: Is this a coding flaw ?
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 ?
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 ?
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 <[EMAIL PROTECTED]>
Re: Is this a coding flaw ?
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 XMLReader object using default > > * SAXParserFactory > > * @return the created XMLReader > > * @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(); > > > > > > > > === > > > > 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 <[EMAIL PROTECTED]>
Re: Is this a coding flaw ?
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 XMLReader object using default > * SAXParserFactory > * @return the created XMLReader > * @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(); > > > > === > > 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
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 XMLReader object using default * SAXParserFactory * @return the created XMLReader * @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(); === 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 <[EMAIL PROTECTED]>