Hi,
This afternoon I found a rather critical performance bug in the flash
generator in MMbase. The idea is as follows:
1. User does a request for a flash-generated page
2. Servflash.request() is called
3. MMFlash module is called to do all the work
4. Servflash.request() returns data and exists
The problem: the request() method is synchronized, meaning that only ONE
thread can generate flash, all others must wait. With one of our sites
this had the effect that if one client does a difficult search (taking
1.5 minutes), the entire site was unreachable during that time. When
somebody requested any page, the flash generator was triggered, but
since there already was a thread in the Servflash.request() method, he
had to wait.
I have fixed this the following way:
- nothing in servflash and mmflash is synchronized anymore
- mmflash used to always use the same filename (temppath +
"import.sws"), this filename is now randomized so that concurrent
instances of MMFlash do not interfere with eachother.
I have attached the unified-diff for this problem. I have also fixed
several other issues in MMFlash:
- general code cleanup (diff doesn't show since I diffed with -w) to
comply with coding standards
- comments for all methods
- several instances of '/' were changed into 'File.separator'
- better checking on the specified value of 'generatortemppath'
The one issue I haven't implemented yet is cleanup of all temporary
data, I will add this before checking it in. I also want to check it in
into the MMBase 1.5 branch since we are going to run this version of
mmbase for the website in question. Since it is a performance bugfix, I
think it is allowed to be checked in, but I wanted to make that sure
before committing my files.
If nobody objects, I'll check it in somewhere next week.
Johannes
Index: module/gui/flash/MMFlash.java
===================================================================
RCS file: /usr/local/cvs/mmbase/module/gui/flash/MMFlash.java,v
retrieving revision 1.13.2.1
diff -w -d -u -r1.13.2.1 MMFlash.java
--- MMFlash.java 2002/03/01 14:02:38 1.13.2.1
+++ MMFlash.java 2002/09/18 14:15:48
@@ -47,6 +47,9 @@
LRUHashtable lru=new LRUHashtable(128);
MMBase mmb;
+ /**
+ * Initialize the module, check if all given parameters (in the XML file) are correct
+ */
public void init() {
htmlroot = MMBaseContext.getHtmlRoot();
mmb=(MMBase)getModule("MMBASEROOT");
@@ -63,7 +66,7 @@
// check if we may create a file on location of generatorTempPath
File tempPath = new File(generatortemppath);
if(!tempPath.isDirectory()) {
- log.error("Generator Temp Path was not a direcory('"+generatorpath+generatorprogram+"'), please edit mmflash.xml, or create directory");
+ log.error("Generator Temp Path was not a direcory('" + generatortemppath + "'), please edit mmflash.xml, or create directory");
}
try {
File test = File.createTempFile("flash", "test", tempPath);
@@ -72,13 +75,17 @@
log.error("Could not create a temp file in directory:'"+generatortemppath+"' for flash, please edit mmflash.xml or change rights");
}
+ if (!generatortemppath.endsWith(File.separator)) {
+ generatortemppath += File.separator;
+ }
+
// check if there is a program on this location
try {
(Runtime.getRuntime()).exec(generatorpath+generatorprogram);
} catch (Exception e) {
log.error("Could not execute command:'"+generatorpath+generatorprogram+"' for flash, please edit mmflash.xml");
}
- log.debug("Module MMFlash started (flash-generator='"+generatorpath+generatorprogram+"' and can be executed and tmpdir is checked)");
+ log.debug("Module MMFlash started (flash-generator='" + generatorpath + generatorprogram + "' can be executed and tmpdir is checked)");
}
public void onload() {
@@ -87,13 +94,31 @@
public MMFlash() {
}
- public synchronized byte[] getDebugSwt(scanpage sp) {
+ /**
+ * Return the generated flash with debug information.<br>
+ * @param sp the scanpage that includes the parameters needed to generate the flash
+ * @return array of bytes containing the flash data
+ */
+ public byte[] getDebugSwt(scanpage sp) {
String filename=htmlroot+sp.req.getRequestURI();
byte[] bytes=generateSwtDebug(filename);
- return(bytes);
+ return bytes;
}
- public synchronized byte[] getScanParsedFlash(scanpage sp) {
+ /**
+ * Return the generated flash. <br>
+ * First, the XML file that does the transformation is being read into an XML reader. Then
+ * all operations for the flash-generator are saved into a file 'input.sws'. Then the
+ * swift-generator is executed on this file, generating a 'export.swf'. This file is read
+ * into a byte array and returned as a result of this method.
+ * @param sp the scanpage that includes the parameters needed to generate the flash
+ * @return array of bytes containing the flash data
+ */
+ public byte[] getScanParsedFlash(scanpage sp) {
+ // First of all, to allow concurrent requests to this method, we define a prefix that we will
+ // add to all files involved.
+ String randomprefix = "" + (new Random()).nextLong();
+
// Get inputfile
String url = sp.req.getRequestURI();
String filename=htmlroot+url;
@@ -127,8 +152,7 @@
src=purl+src;
body+="INPUT \""+htmlroot+src+"\"\n";
}
- body+="OUTPUT \""+generatortemppath+"export.swf\"\n";
-
+ body += "OUTPUT \"" + generatortemppath + randomprefix + "export.swf\"\n";
// is there a caching option set ?
String caching=script.getCaching();
@@ -154,21 +178,20 @@
}
}
}
- }// !sp.reload
-
+ }
String scriptpath=src;
scriptpath=scriptpath.substring(0,scriptpath.lastIndexOf('/')+1);
- body+=addDefines(script.getDefines(),scriptpath);
+ body += addDefines(script.getDefines(), scriptpath, randomprefix);
body+=addReplaces(script.getReplaces(),scriptpath);
// save the created input file for the generator
- saveFile(generatortemppath+"input.sws",body);
+ saveFile(generatortemppath + randomprefix + "input.sws", body);
// lets generate the file
- generateFlash(scriptpath);
+ generateFlash(scriptpath, generatortemppath + randomprefix + "input.sws");
- byte[] bytes=readBytesFile(generatortemppath+"export.swf");
+ byte[] bytes = readBytesFile(generatortemppath + randomprefix + "export.swf");
if (caching!=null && caching.equals("lru")) {
lru.put(url+query,bytes);
} else if (caching!=null && caching.equals("disk")) {
@@ -189,7 +212,11 @@
* other things, like pictures.(THIS LOOKS BELOW THE mmbase.htmlroot !!)
* @return a byte thingie, which contains the new generated flash thingie
*/
- public synchronized byte[] getParsedFlash(String flashXML, String workingdir) {
+ public byte[] getParsedFlash(String flashXML, String workingdir) {
+ // First of all, to allow concurrent requests to this method, we define a prefix that we will
+ // add to all files involved.
+ String randomprefix = "" + (new Random()).nextLong();
+
CharArrayReader reader=new CharArrayReader(flashXML.toCharArray());
XMLDynamicFlashReader script=new XMLDynamicFlashReader(reader);
String body="";
@@ -231,22 +258,22 @@
// hey ho, generate our template..
body+="INPUT \""+inputFile.getAbsolutePath()+"\"\n";
- body+="OUTPUT \""+generatortemppath+"export.swf\"\n";
+ body += "OUTPUT \"" + generatortemppath + randomprefix + "export.swf\"\n";
String scriptpath=src;
scriptpath=scriptpath.substring(0,scriptpath.lastIndexOf('/')+1);
- body+=addDefines(script.getDefines(),scriptpath);
+ body += addDefines(script.getDefines(), scriptpath, randomprefix);
body+=addReplaces(script.getReplaces(),scriptpath);
// save the created input file for the generator
- saveFile(generatortemppath+"input.sws",body);
+ saveFile(generatortemppath + randomprefix + "input.sws", body);
// lets generate the file
- generateFlash(scriptpath);
+ generateFlash(scriptpath, generatortemppath + randomprefix + "input.sws");
// retrieve the result of the genererator..
- byte[] bytes=readBytesFile(generatortemppath+"export.swf");
+ byte[] bytes = readBytesFile(generatortemppath + randomprefix + "export.swf");
// store the flash in cache, when needed...
if (caching!=null && (caching.equals("lru")|| caching.equals("disk")) ) {
@@ -258,6 +285,11 @@
return(bytes);
}
+ /**
+ * Generate text to add to the swift-genertor input file. This text specifies
+ * how the flash should be manipulated. It allows replacements of colors,
+ * fontsizes, etc.
+ */
private String addReplaces(Vector replaces, String scriptpath) {
String part="";
for (Enumeration e=replaces.elements();e.hasMoreElements();) {
@@ -337,11 +369,16 @@
}
part+="\n";
}
- return(part);
+ return part;
}
- private String addDefines(Vector defines,String scriptpath) {
+ /**
+ * Add all defined media files (sound, images, etc.) to the text
+ * that is used for the swift-generator. Images that come from inside
+ * MMBase are saved to disk using a random prefix.
+ */
+ private String addDefines(Vector defines, String scriptpath, String randomprefix) {
String part="";
int counter=1;
for (Enumeration e=defines.elements();e.hasMoreElements();) {
@@ -358,7 +395,7 @@
String src=(String)rep.get("src");
if (src!=null) {
if (src.startsWith("/img.db?")) {
- String result=mapImage(src.substring(8),counter++);
+ String result = mapImage(src.substring(8), counter++, randomprefix);
part+=" \""+result+"\"";
} else if (src.startsWith("/")) {
part+=" \""+htmlroot+src+"\"";
@@ -403,10 +440,15 @@
}
part+="\n";
}
- return(part);
+ return part;
}
- byte[] readBytesFile(String filename) {
+ /**
+ * Read binary data from a file
+ * @param filename File to read data from
+ * @return bytearray containing the data
+ */
+ private byte[] readBytesFile(String filename) {
File bfile = new File(filename);
int filesize = (int)bfile.length();
byte[] buffer=new byte[filesize];
@@ -425,6 +467,12 @@
}
+ /**
+ * Load the flash data from disk for a given filename with it's query
+ * @param filename Filename to read from disk
+ * @param query Querystring for this filename
+ * @return bytearray containing the flash data
+ */
private byte[] loadDiskCache(String filename,String query) {
if (query!=null) {
filename=filename.substring(0,filename.length()-3)+"swf?"+query;
@@ -455,6 +503,11 @@
}
+ /**
+ * Generate a flash file for a given input filename and return the data as a bytearray
+ * @param filename File to generator flash for
+ * @return byte-array containing the data
+ */
private byte[] generateSwtDebug(String filename) {
Process p=null;
String s="",tmp="";
@@ -472,7 +525,6 @@
s+=e.toString();
out.print(s);
}
- log.debug("Executed command: "+command+" succesfull, now gonna parse");
log.info("Executed command: "+command+" succesfull, now gonna parse");
dip = new DataInputStream(new BufferedInputStream(p.getInputStream()));
byte[] result=new byte[32000];
@@ -505,7 +557,12 @@
return(result);
}
- private void generateFlash(String scriptpath) {
+ /**
+ * Generate a flash file for a given input filename
+ * @param scriptpath Unused parameter
+ * @param inputfile File to generate flash for
+ */
+ private void generateFlash(String scriptpath, String inputfile) {
Process p=null;
String s="",tmp="";
DataInputStream dip= null;
@@ -515,7 +572,7 @@
RandomAccessFile dos=null;
try {
- command=generatorpath+generatorprogram+" "+generatortemppath+"input.sws";
+ command = generatorpath + generatorprogram + " " + inputfile;
p = (Runtime.getRuntime()).exec(command);
} catch (Exception e) {
log.error("could not execute command:'"+command+"'");
@@ -527,6 +584,7 @@
byte[] result=new byte[1024];
// look on the input stream
+ // WHY???
try {
int len3=0;
int len2=0;
@@ -550,20 +608,34 @@
}
}
- static boolean saveFile(String filename,String value) {
+ /**
+ * Save a stringvalue to a file on the filesystem
+ * @param filename File to save the stringvalue to
+ * @param value Value to save to disk
+ * @return Boolean indicating succes
+ */
+ private boolean saveFile(String filename, String value) {
File sfile = new File(filename);
try {
DataOutputStream scan = new DataOutputStream(new FileOutputStream(sfile));
scan.writeBytes(value);
scan.flush();
scan.close();
+ return true;
} catch(Exception e) {
log.error("Could not write values to file:" +filename+ " with value" + value);
e.printStackTrace();
+ return false;
}
- return(true);
}
+ /**
+ * Save the binary data to the disk cache<br>
+ * <b>TODO<b>: This assumes that the filename has a 3-character long extension, and that '?' characters are allowed in filenames
+ * @param filename The filename to save the data to
+ * @param value The binary data
+ * @param query The querystring to add to the filename
+ */
private boolean saveDiskCache(String filename,String query,byte[] value) {
if (query!=null) {
filename=filename.substring(0,filename.length()-3)+"swf?"+query;
@@ -573,16 +645,15 @@
if (subdir!=null && !subdir.equals("")) {
int pos=filename.lastIndexOf('/');
- filename=filename.substring(0,pos)+"/"+subdir+filename.substring(pos);
+ filename = filename.substring(0, pos) + File.separator + subdir + filename.substring(pos);
// Create dir if it doesn't exist
- File d=new File(filename.substring(0,pos)+"/"+subdir);
+ File d = new File(filename.substring(0, pos) + File.separator + subdir);
if (!d.exists()) {
d.mkdir();
}
}
log.debug("filename="+filename);
- //System.out.println("filename="+filename);
File sfile = new File(filename);
try {
@@ -597,7 +668,14 @@
return(true);
}
- String mapImage(String imageline,int counter) {
+ /**
+ * Get an image from MMBase given a list of parameters, and save it to disk
+ * @param imageline The image number with it's manipulations (eg: 34235+s(50))
+ * @param counter The number to prefix to the image
+ * @param randomprefix A prefix for the image filename
+ * @return The complete path to the image
+ */
+ private String mapImage(String imageline, int counter, String prefix) {
Images bul=(Images)mmb.getMMObject("images");
Vector params=new Vector();
if (bul!=null) {
@@ -608,35 +686,43 @@
params.addElement(tok.nextToken());
scanpage sp=new scanpage();
byte[] bytes=bul.getImageBytes(sp,params);
- saveFile(generatortemppath+"/image"+counter+".jpg",bytes);
+ saveFile(generatortemppath + prefix + "image" + counter + ".jpg", bytes);
}
+ } else {
+ log.error("Cannot locate images builder, make sure you activated it!");
}
- return(generatortemppath+"/image"+counter+".jpg");
+ return(generatortemppath + prefix + "image" + counter + ".jpg");
}
-
- static boolean saveFile(String filename,byte[] value) {
+ /**
+ * Save a byte-array to disk
+ * @param filename The name of the file to save the data to
+ * @param value The byte-array containing the data for the file
+ * @return Boolean indicating the success of the operation
+ */
+ private boolean saveFile(String filename, byte[] value) {
File sfile = new File(filename);
try {
DataOutputStream scan = new DataOutputStream(new FileOutputStream(sfile));
scan.write(value);
scan.flush();
scan.close();
+ return true;
} catch(Exception e) {
log.error("Could not save to file:"+filename);
log.error(Logging.stackTrace(e));
+ return false;
}
- return(true);
}
/**
* Escape quotes in a string, because flash generator will fail otherwise
+ * @param unquoted The string with quotes (") in it
+ * @return The string where all quotes are escaped as \"
*/
- String replaceQuote(String unquoted) {
- log.debug("Starting to replace a quote ... ");
+ private String replaceQuote(String unquoted) {
StringObject so = new StringObject(unquoted);
so.replace("\"", "\\\"");
- log.debug("Finished ...");
return so.toString();
}
}
Index: module/gui/flash/XMLDynamicFlashReader.java
===================================================================
RCS file: /usr/local/cvs/mmbase/module/gui/flash/XMLDynamicFlashReader.java,v
retrieving revision 1.3
diff -w -d -u -r1.3 XMLDynamicFlashReader.java
Index: servlet/servflash.java
===================================================================
RCS file: /usr/local/cvs/mmbase/servlet/servflash.java,v
retrieving revision 1.6.2.1
diff -w -d -u -r1.6.2.1 servflash.java
--- servflash.java 2002/03/21 16:37:31 1.6.2.1
+++ servflash.java 2002/09/18 14:15:48
@@ -53,7 +53,7 @@
* service call will be called by the server when a request is done
* by a user.
*/
- public synchronized void service(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException {
+ public void service(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException {
incRefCount(req);
try {
log.service("Parsing FLASH page: " + req.getRequestURI());
