RFC: Patch for Bug#749 (sun.applet.PluginStreamHandler#handleMessage(String) really slow)
Deepak Bhole
dbhole at redhat.com
Tue Jul 5 08:16:16 PDT 2011
* Andrew John Hughes <ahughes at redhat.com> [2011-07-04 21:48]:
> On Mon, Jul 04, 2011 at 06:35:14PM -0400, Deepak Bhole wrote:
> > Hi,
> >
> > This is a cleaned up version of the patch posted for Bug# 749:
> > http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=749
> >
> > I have reviewed the patch myself and found no issues. The patch is
> > fairly straightforward and if there are no emails by tomorrow,
> > I will go ahead and commit to 1.0, 1.1 and HEAD.
> >
> > Cheers,
> > Deepak
>
> I've commented inline. The code itself looks ok, but it needs to be
> better documented.
>
> However, your comment about applying this patch, given no response
> disturbs me. We don't have a policy of doing this and I strongly
> think we should not do so. Please wait for a review, especially
> for release branch patches. You can't take no response after some
> short period (and 'tomorrow' is quite vague) as implicit approval
> of your patch. People do have other stuff to work on.
>
The fix looked quite harmless so I didn't think much of it. Nonetheless,
I will wait for approval next time.
New patch with updated docs attached. Thanks for taking a look!
Cheers,
Deepak
> > diff -r 86abbf8be0b1 AUTHORS
> > --- a/AUTHORS Thu Jun 23 15:29:45 2011 +0200
> > +++ b/AUTHORS Mon Jul 04 18:29:15 2011 -0400
> > @@ -3,6 +3,7 @@
> >
> > Lillian Angel <langel at redhat.com>
> > Deepak Bhole <dbhole at redhat.com>
> > +Ricardo Martín Camarero <rickyepoderi at yahoo.es>
> > Thomas Fitzsimmons <fitzsim at redhat.com>
> > Mark Greenwood <mark at dcs.shef.ac.uk>
> > Andrew John Hughes <gnu_andrew at member.fsf.org, ahughes at redhat.com>
> > diff -r 86abbf8be0b1 ChangeLog
> > --- a/ChangeLog Thu Jun 23 15:29:45 2011 +0200
> > +++ b/ChangeLog Mon Jul 04 18:29:15 2011 -0400
> > @@ -1,3 +1,12 @@
> > +2011-07-04 Deepak Bhole <dbhole at redhat.com>
> > +
> > + PR749: sun.applet.PluginStreamHandler#handleMessage(String) really slow
> > + Patch from: Ricardo Martín Camarero (Ricky) <rickyepoderi at yahoo dot es>
> > + * plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
> > + (readPair): New function.
> > + (handleMessage): Use readPair to incrementally tokenize message, rather
> > + than using String.split().
> > +
> > 2011-06-22 Jiri Vanek <jvanek at redhat.com>
> >
> > * tests/report-styles/jreport.xsl: part with classes statistics is now collapsable
> > diff -r 86abbf8be0b1 NEWS
> > --- a/NEWS Thu Jun 23 15:29:45 2011 +0200
> > +++ b/NEWS Mon Jul 04 18:29:15 2011 -0400
> > @@ -8,6 +8,10 @@
> >
> > CVE-XXXX-YYYY: http://www.cve.mitre.org/cgi-bin/cvename.cgi?name=XXXX-YYYY
> >
> > +New in release 1.2 (2011-XX-XX):
> > +* Plugin
> > + - PR749: sun.applet.PluginStreamHandler#handleMessage(String) really slow
> > +
> > New in release 1.1 (2011-XX-XX):
> > * Security updates
> > - S6983554, CVE-2010-4450: Launcher incorrect processing of empty library path entries
> > diff -r 86abbf8be0b1 plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
> > --- a/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Thu Jun 23 15:29:45 2011 +0200
> > +++ b/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Mon Jul 04 18:29:15 2011 -0400
> > @@ -113,18 +113,60 @@
> > listenerThread.start();
> > }
> >
> > + /**
> > + * Given a string, separates the first (space separated) token and puts the
> > + * rest into another string.
> > + *
> > + * @param message The string to read
> > + * @param start The position to start reading at
> > + * @param array The array into which the first token and rest are to be placed
> > + * @return Position where the next token starts
> > + */
>
> This documentation is inaccurate. It puts a string up to the first space into the
> first element of the array, and then a string up to the second space into the
> second element of the array.
>
> So in the following cases:
>
> "foo" -> array[0] = "foo", array[1] = null
> "foo bar" -> array[0] = "foo", array[1] = "bar"
> "foo bar baz" -> array[0] = "foo", array[1] = "bar"
>
> Note that the second and third examples have identical output and anything after the
> second space is discarded.
>
> It wasn't immediately clear why this is so different to what split was
> doing. Reading the bug it seems this is done for performance reasons
> (so a regexp is not matched against a long string, creating a huge
> array). This should be documented in the Javadoc and probably in the
> Changelog too.
>
> > +
> > + private int readPair(String message, int start, String[] array) {
> > +
> > + int end = start;
> > + array[0] = null;
> > + array[1] = null;
> > +
> > + if (message.length() > start) {
> > + int firstSpace = message.indexOf(' ', start);
> > + if (firstSpace == -1) {
> > + array[0] = message.substring(start);
> > + end = message.length();
> > + } else {
> > + array[0] = message.substring(start, firstSpace);
> > + if (message.length() > firstSpace + 1) {
> > + int secondSpace = message.indexOf(' ', firstSpace + 1);
> > + if (secondSpace == -1) {
> > + array[1] = message.substring(firstSpace + 1);
> > + end = message.length();
> > + } else {
> > + array[1] = message.substring(firstSpace + 1, secondSpace);
> > + end = secondSpace + 1;
> > + }
> > + }
> > + }
> > + }
> > +
> > + PluginDebug.debug("readPair: '", array[0], "' - '", array[1], "' ", end);
> > + return end;
> > + }
> > +
> > public void handleMessage(String message) throws PluginException {
> >
> > - int nextIndex = 0;
> > int reference = -1;
> > String src = null;
> > String[] privileges = null;
> > String rest = "";
> > + String[] msgComponents = new String[2];
> > + int pos = 0;
> > + int oldPos = 0;
> >
> > - String[] msgComponents = message.split(" ");
> > -
> > - if (msgComponents.length < 2)
> > + pos = readPair(message, oldPos, msgComponents);
> > + if (msgComponents[0] == null || msgComponents[1] == null) {
> > return;
> > + }
> >
> > if (msgComponents[0].startsWith("plugin")) {
> > handlePluginMessage(message);
> > @@ -134,38 +176,38 @@
> > // type and identifier are guaranteed to be there
> > String type = msgComponents[0];
> > final int identifier = Integer.parseInt(msgComponents[1]);
> > - nextIndex = 2;
> >
> > // reference, src and privileges are optional components,
> > // and are guaranteed to be in that order, if they occur
> > + oldPos = pos;
> > + pos = readPair(message, oldPos, msgComponents);
> >
> > // is there a reference ?
> > - if (msgComponents[nextIndex].equals("reference")) {
> > - reference = Integer.parseInt(msgComponents[nextIndex + 1]);
> > - nextIndex += 2;
> > + if ("reference".equals(msgComponents[0])) {
> > + reference = Integer.parseInt(msgComponents[1]);
> > + oldPos = pos;
> > + pos = readPair(message, oldPos, msgComponents);
> > }
> >
> > // is there a src?
> > - if (msgComponents[nextIndex].equals("src")) {
> > - src = msgComponents[nextIndex + 1];
> > - nextIndex += 2;
> > + if ("src".equals(msgComponents[0])) {
> > + src = msgComponents[1];
> > + oldPos = pos;
> > + pos = readPair(message, oldPos, msgComponents);
> > }
> >
> > // is there a privileges?
> > - if (msgComponents[nextIndex].equals("privileges")) {
> > - String privs = msgComponents[nextIndex + 1];
> > + if ("privileges".equals(msgComponents[0])) {
> > + String privs = msgComponents[1];
> > privileges = privs.split(",");
> > - nextIndex += 2;
> > + oldPos = pos;
> > }
> >
> > // rest
> > - for (int i = nextIndex; i < msgComponents.length; i++) {
> > - rest += msgComponents[i];
> > - rest += " ";
> > + if (message.length() > oldPos) {
> > + rest = message.substring(oldPos);
> > }
> >
> > - rest = rest.trim();
> > -
> > try {
> >
> > PluginDebug.debug("Breakdown -- type: ", type, " identifier: ", identifier, " reference: ", reference, " src: ", src, " privileges: ", privileges, " rest: \"", rest, "\"");
>
>
> --
> Andrew :)
>
> Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
>
> Support Free Java!
> Contribute to GNU Classpath and IcedTea
> http://www.gnu.org/software/classpath
> http://icedtea.classpath.org
> PGP Key: F5862A37 (https://keys.indymedia.org/)
> Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
-------------- next part --------------
diff -r 86abbf8be0b1 AUTHORS
--- a/AUTHORS Thu Jun 23 15:29:45 2011 +0200
+++ b/AUTHORS Tue Jul 05 11:12:20 2011 -0400
@@ -3,6 +3,7 @@
Lillian Angel <langel at redhat.com>
Deepak Bhole <dbhole at redhat.com>
+Ricardo Martín Camarero <rickyepoderi at yahoo.es>
Thomas Fitzsimmons <fitzsim at redhat.com>
Mark Greenwood <mark at dcs.shef.ac.uk>
Andrew John Hughes <gnu_andrew at member.fsf.org, ahughes at redhat.com>
diff -r 86abbf8be0b1 ChangeLog
--- a/ChangeLog Thu Jun 23 15:29:45 2011 +0200
+++ b/ChangeLog Tue Jul 05 11:12:20 2011 -0400
@@ -1,3 +1,12 @@
+2011-07-05 Deepak Bhole <dbhole at redhat.com>
+
+ PR749: sun.applet.PluginStreamHandler#handleMessage(String) really slow
+ Patch from: Ricardo Martín Camarero (Ricky) <rickyepoderi at yahoo dot es>
+ * plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
+ (readPair): New function.
+ (handleMessage): Use readPair to incrementally tokenize message, rather
+ than using String.split().
+
2011-06-22 Jiri Vanek <jvanek at redhat.com>
* tests/report-styles/jreport.xsl: part with classes statistics is now collapsable
diff -r 86abbf8be0b1 NEWS
--- a/NEWS Thu Jun 23 15:29:45 2011 +0200
+++ b/NEWS Tue Jul 05 11:12:20 2011 -0400
@@ -8,6 +8,10 @@
CVE-XXXX-YYYY: http://www.cve.mitre.org/cgi-bin/cvename.cgi?name=XXXX-YYYY
+New in release 1.2 (2011-XX-XX):
+* Plugin
+ - PR749: sun.applet.PluginStreamHandler#handleMessage(String) really slow
+
New in release 1.1 (2011-XX-XX):
* Security updates
- S6983554, CVE-2010-4450: Launcher incorrect processing of empty library path entries
diff -r 86abbf8be0b1 plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
--- a/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Thu Jun 23 15:29:45 2011 +0200
+++ b/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java Tue Jul 05 11:12:20 2011 -0400
@@ -113,18 +113,58 @@
listenerThread.start();
}
+ /**
+ * Given a string, reads the first two (space separated) tokens.
+ *
+ * @param message The string to read
+ * @param start The position to start reading at
+ * @param array The array into which the first two tokens are placed
+ * @return Position where the next token starts
+ */
+ private int readPair(String message, int start, String[] array) {
+
+ int end = start;
+ array[0] = null;
+ array[1] = null;
+
+ if (message.length() > start) {
+ int firstSpace = message.indexOf(' ', start);
+ if (firstSpace == -1) {
+ array[0] = message.substring(start);
+ end = message.length();
+ } else {
+ array[0] = message.substring(start, firstSpace);
+ if (message.length() > firstSpace + 1) {
+ int secondSpace = message.indexOf(' ', firstSpace + 1);
+ if (secondSpace == -1) {
+ array[1] = message.substring(firstSpace + 1);
+ end = message.length();
+ } else {
+ array[1] = message.substring(firstSpace + 1, secondSpace);
+ end = secondSpace + 1;
+ }
+ }
+ }
+ }
+
+ PluginDebug.debug("readPair: '", array[0], "' - '", array[1], "' ", end);
+ return end;
+ }
+
public void handleMessage(String message) throws PluginException {
- int nextIndex = 0;
int reference = -1;
String src = null;
String[] privileges = null;
String rest = "";
+ String[] msgComponents = new String[2];
+ int pos = 0;
+ int oldPos = 0;
- String[] msgComponents = message.split(" ");
-
- if (msgComponents.length < 2)
+ pos = readPair(message, oldPos, msgComponents);
+ if (msgComponents[0] == null || msgComponents[1] == null) {
return;
+ }
if (msgComponents[0].startsWith("plugin")) {
handlePluginMessage(message);
@@ -134,38 +174,38 @@
// type and identifier are guaranteed to be there
String type = msgComponents[0];
final int identifier = Integer.parseInt(msgComponents[1]);
- nextIndex = 2;
// reference, src and privileges are optional components,
// and are guaranteed to be in that order, if they occur
+ oldPos = pos;
+ pos = readPair(message, oldPos, msgComponents);
// is there a reference ?
- if (msgComponents[nextIndex].equals("reference")) {
- reference = Integer.parseInt(msgComponents[nextIndex + 1]);
- nextIndex += 2;
+ if ("reference".equals(msgComponents[0])) {
+ reference = Integer.parseInt(msgComponents[1]);
+ oldPos = pos;
+ pos = readPair(message, oldPos, msgComponents);
}
// is there a src?
- if (msgComponents[nextIndex].equals("src")) {
- src = msgComponents[nextIndex + 1];
- nextIndex += 2;
+ if ("src".equals(msgComponents[0])) {
+ src = msgComponents[1];
+ oldPos = pos;
+ pos = readPair(message, oldPos, msgComponents);
}
// is there a privileges?
- if (msgComponents[nextIndex].equals("privileges")) {
- String privs = msgComponents[nextIndex + 1];
+ if ("privileges".equals(msgComponents[0])) {
+ String privs = msgComponents[1];
privileges = privs.split(",");
- nextIndex += 2;
+ oldPos = pos;
}
// rest
- for (int i = nextIndex; i < msgComponents.length; i++) {
- rest += msgComponents[i];
- rest += " ";
+ if (message.length() > oldPos) {
+ rest = message.substring(oldPos);
}
- rest = rest.trim();
-
try {
PluginDebug.debug("Breakdown -- type: ", type, " identifier: ", identifier, " reference: ", reference, " src: ", src, " privileges: ", privileges, " rest: \"", rest, "\"");
More information about the distro-pkg-dev
mailing list