RFC: Patch for Bug#749 (sun.applet.PluginStreamHandler#handleMessage(String) really slow)
Andrew John Hughes
ahughes at redhat.com
Mon Jul 4 18:48:10 PDT 2011
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.
> 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
More information about the distro-pkg-dev
mailing list