Re: [xwiki-devs] [xwiki-notifications] r34247 - platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl
On Jan 29, 2011, at 3:59 AM, sdumitriu (SVN) wrote:
Author: sdumitriu Date: 2011-01-29 03:59:38 +0100 (Sat, 29 Jan 2011) New Revision: 34247
Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java Log: [refactoring] Better performance by avoiding repeated lookup of components
Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java 2011-01-29 02:52:13 UTC (rev 34246) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java 2011-01-29 02:59:38 UTC (rev 34247) @@ -124,6 +124,12 @@ /** Logging helper object. */ private static final Log LOG = LogFactory.getLog(PdfExportImpl.class);
+ /** Velocity engine manager, used for interpreting velocity. */ + private static VelocityManager velocityManager = Utils.getComponent(VelocityManager.class); + + /** The OpenOffice manager used for generating RTF. */ + private static OpenOfficeManager oooManager = Utils.getComponent(OpenOfficeManager.class);
This is not very since it will prevent the Extension Manager and the dynamic component mechanism to work (if a new component impl overrides the role/hint). We shouldn't use any static as much as possible. Note that a lookup doesn't cost anything since there's no synchronize and it's only a concurrent hashmap get, so I doubt it's going to improve perf by much (unless there's something I don't understand). Note that in the future an idea we've had with Thomas is that all @Requirement will actually be proxies to the component impl so that if the component is replaced you'll always get the current one. Thanks -Vincent
+ /** DOM parser factory. */ private static DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
@@ -242,11 +248,9 @@ LOG.debug("Final XHTML for export: " + xhtml); }
- OpenOfficeManager OpenOfficeManager = Utils.getComponent(OpenOfficeManager.class); - // If OpenOffice server is connected use it instead of FOP which does not support RTF very well // Only switch to openoffice server for RTF because FOP is supposedly a lot more powerful for PDF - if (type != PDF && OpenOfficeManager.getState() == ManagerState.CONNECTED) { + if (type != PDF && oooManager.getState() == ManagerState.CONNECTED) { exportOffice(xhtml, out, type, context); } else { // XSL Transformation to XML-FO @@ -285,10 +289,8 @@ // id attribute on body element makes openoffice converter to fail html = html.replaceFirst("(<body[^>]+)id=\"body\"([^>]*>)", "$1$2");
- OpenOfficeManager OpenOfficeManager = Utils.getComponent(OpenOfficeManager.class); + OpenOfficeConverter documentConverter = oooManager.getConverter();
- OpenOfficeConverter documentConverter = OpenOfficeManager.getConverter(); - String inputFileName = "export_input.html"; String outputFileName = "export_output" + (type == PdfExportImpl.RTF ? ".rtf" : ".pdf");
@@ -605,7 +607,6 @@ Utils.getComponent(EntityReferenceSerializer.class); try { StringWriter writer = new StringWriter(); - VelocityManager velocityManager = Utils.getComponent(VelocityManager.class); VelocityEngine engine = velocityManager.getVelocityEngine(); try { VelocityContext vcontext = velocityManager.getVelocityContext();
On 01/29/2011 08:54 AM, Vincent Massol wrote:
On Jan 29, 2011, at 3:59 AM, sdumitriu (SVN) wrote:
Author: sdumitriu Date: 2011-01-29 03:59:38 +0100 (Sat, 29 Jan 2011) New Revision: 34247
Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java Log: [refactoring] Better performance by avoiding repeated lookup of components
Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java 2011-01-29 02:52:13 UTC (rev 34246) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java 2011-01-29 02:59:38 UTC (rev 34247) @@ -124,6 +124,12 @@ /** Logging helper object. */ private static final Log LOG = LogFactory.getLog(PdfExportImpl.class);
+ /** Velocity engine manager, used for interpreting velocity. */ + private static VelocityManager velocityManager = Utils.getComponent(VelocityManager.class); + + /** The OpenOffice manager used for generating RTF. */ + private static OpenOfficeManager oooManager = Utils.getComponent(OpenOfficeManager.class);
This is not very since it will prevent the Extension Manager and the dynamic component mechanism to work (if a new component impl overrides the role/hint).
OK, I see, but this is not something done with the current component injection, since an @Requirement doesn't change after the singleton instance is initialized. I made this change preparing for a future component, where the only change would be to replace the Utils.getComponent call with @Requirement. Since the PdfExportImpl class is a singleton as well, there's no difference between static and instance at the moment. If you want, i can remove the static declaration.
We shouldn't use any static as much as possible. Note that a lookup doesn't cost anything since there's no synchronize and it's only a concurrent hashmap get, so I doubt it's going to improve perf by much (unless there's something I don't understand).
Note that in the future an idea we've had with Thomas is that all @Requirement will actually be proxies to the component impl so that if the component is replaced you'll always get the current one.
Yes, this looks nice.
Thanks -Vincent
+ /** DOM parser factory. */ private static DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
@@ -242,11 +248,9 @@ LOG.debug("Final XHTML for export: " + xhtml); }
- OpenOfficeManager OpenOfficeManager = Utils.getComponent(OpenOfficeManager.class); - // If OpenOffice server is connected use it instead of FOP which does not support RTF very well // Only switch to openoffice server for RTF because FOP is supposedly a lot more powerful for PDF - if (type != PDF&& OpenOfficeManager.getState() == ManagerState.CONNECTED) { + if (type != PDF&& oooManager.getState() == ManagerState.CONNECTED) { exportOffice(xhtml, out, type, context); } else { // XSL Transformation to XML-FO @@ -285,10 +289,8 @@ // id attribute on body element makes openoffice converter to fail html = html.replaceFirst("(<body[^>]+)id=\"body\"([^>]*>)", "$1$2");
- OpenOfficeManager OpenOfficeManager = Utils.getComponent(OpenOfficeManager.class); + OpenOfficeConverter documentConverter = oooManager.getConverter();
- OpenOfficeConverter documentConverter = OpenOfficeManager.getConverter(); - String inputFileName = "export_input.html"; String outputFileName = "export_output" + (type == PdfExportImpl.RTF ? ".rtf" : ".pdf");
@@ -605,7 +607,6 @@ Utils.getComponent(EntityReferenceSerializer.class); try { StringWriter writer = new StringWriter(); - VelocityManager velocityManager = Utils.getComponent(VelocityManager.class); VelocityEngine engine = velocityManager.getVelocityEngine(); try { VelocityContext vcontext = velocityManager.getVelocityContext();
-- Sergiu Dumitriu http://purl.org/net/sergiu/
On Jan 29, 2011, at 2:33 PM, Sergiu Dumitriu wrote:
On 01/29/2011 08:54 AM, Vincent Massol wrote:
On Jan 29, 2011, at 3:59 AM, sdumitriu (SVN) wrote:
Author: sdumitriu Date: 2011-01-29 03:59:38 +0100 (Sat, 29 Jan 2011) New Revision: 34247
Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java Log: [refactoring] Better performance by avoiding repeated lookup of components
Modified: platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java =================================================================== --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java 2011-01-29 02:52:13 UTC (rev 34246) +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/pdf/impl/PdfExportImpl.java 2011-01-29 02:59:38 UTC (rev 34247) @@ -124,6 +124,12 @@ /** Logging helper object. */ private static final Log LOG = LogFactory.getLog(PdfExportImpl.class);
+ /** Velocity engine manager, used for interpreting velocity. */ + private static VelocityManager velocityManager = Utils.getComponent(VelocityManager.class); + + /** The OpenOffice manager used for generating RTF. */ + private static OpenOfficeManager oooManager = Utils.getComponent(OpenOfficeManager.class);
This is not very since it will prevent the Extension Manager and the dynamic component mechanism to work (if a new component impl overrides the role/hint).
OK, I see, but this is not something done with the current component injection, since an @Requirement doesn't change after the singleton instance is initialized. I made this change preparing for a future component, where the only change would be to replace the Utils.getComponent call with @Requirement.
Since the PdfExportImpl class is a singleton as well, there's no difference between static and instance at the moment. If you want, i can remove the static declaration.
No that's ok. I just wanted to make sure we all understand the limitation and the path to the future. Thanks -Vincent
We shouldn't use any static as much as possible. Note that a lookup doesn't cost anything since there's no synchronize and it's only a concurrent hashmap get, so I doubt it's going to improve perf by much (unless there's something I don't understand).
Note that in the future an idea we've had with Thomas is that all @Requirement will actually be proxies to the component impl so that if the component is replaced you'll always get the current one.
Yes, this looks nice.
Thanks -Vincent
+ /** DOM parser factory. */ private static DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
@@ -242,11 +248,9 @@ LOG.debug("Final XHTML for export: " + xhtml); }
- OpenOfficeManager OpenOfficeManager = Utils.getComponent(OpenOfficeManager.class); - // If OpenOffice server is connected use it instead of FOP which does not support RTF very well // Only switch to openoffice server for RTF because FOP is supposedly a lot more powerful for PDF - if (type != PDF&& OpenOfficeManager.getState() == ManagerState.CONNECTED) { + if (type != PDF&& oooManager.getState() == ManagerState.CONNECTED) { exportOffice(xhtml, out, type, context); } else { // XSL Transformation to XML-FO @@ -285,10 +289,8 @@ // id attribute on body element makes openoffice converter to fail html = html.replaceFirst("(<body[^>]+)id=\"body\"([^>]*>)", "$1$2");
- OpenOfficeManager OpenOfficeManager = Utils.getComponent(OpenOfficeManager.class); + OpenOfficeConverter documentConverter = oooManager.getConverter();
- OpenOfficeConverter documentConverter = OpenOfficeManager.getConverter(); - String inputFileName = "export_input.html"; String outputFileName = "export_output" + (type == PdfExportImpl.RTF ? ".rtf" : ".pdf");
@@ -605,7 +607,6 @@ Utils.getComponent(EntityReferenceSerializer.class); try { StringWriter writer = new StringWriter(); - VelocityManager velocityManager = Utils.getComponent(VelocityManager.class); VelocityEngine engine = velocityManager.getVelocityEngine(); try { VelocityContext vcontext = velocityManager.getVelocityContext();
participants (2)
-
Sergiu Dumitriu -
Vincent Massol