Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance issue with regex in HtmlConverterCoreNodeRenderer #633

Open
praveen-diffbot opened this issue Sep 17, 2024 · 2 comments
Open

Comments

@praveen-diffbot
Copy link

HtmlConverterCoreNodeRenderer.handleTableCell has a call to String.replaceAll("\\s*\n\\s*", " ") which can be quite slow. The regex is quite simple and can be sped up by removing the regex.

To Reproduce

See attached file test.html.txt

public class LoadingTest {
  public static void main(final String[] args) throws Exception {
    final String STR = java.nio.file.Files.readString(java.nio.file.Path.of("test.html.txt"));
    final long tic = System.currentTimeMillis();
    com.diffbot.websearch.html.MarkdownNormalizer.markdown(STR);
    System.out.println("took: " + (System.currentTimeMillis() - tic));
  }
}

Expected behavior
The code takes >4000 ms to run on my laptop.

took: 4024

It should take much lesser time.

@praveen-diffbot
Copy link
Author

I replaced the regex call with an approximation which is probably ok for html

 private String replaceMultipleBlankSpace(String cellText) {
     StringBuilder result = new StringBuilder();
     boolean wasSpace = false;

     for (char c : cellText.toCharArray()) {
         if (Character.isWhitespace(c)) {
             if (!wasSpace) {
                 result.append(' ');
                 wasSpace = true;
             }
         } else {
             result.append(c);
             wasSpace = false;
         }
     }

     return result.toString();
}
String cellText = replaceMultipleBlankSpace(context.processTextNodes(element).trim());

The same file takes about 150ms to process

@DarkTyger
Copy link

DarkTyger commented Oct 14, 2024

Here's a micro test showing the behaviour of the regular expression versus the non-regex solution:

public class T {
  private static final String[] INPUTS = {
    "Line one\nLine two",
    "   Leading spaces\nTrailing spaces   ",
    "Multiple  spaces between words",
    "\n\nBlank lines\n\n",
    "NoSpaces"
  };

  private static final String[] OUTPUTS = {
    "Line one Line two",
    "Leading spaces Trailing spaces",
    "Multiple  spaces between words",
    "Blank lines",
    "NoSpaces"
  };

  public static void main(final String[] args) {
    for (var i = 0; i < INPUTS.length; i++) {
      final var result = regex(INPUTS[i]);

      if (!result.equals(OUTPUTS[i])) {
        System.out.println("Test failed:");
        System.out.println("Input: " + INPUTS[i]);
        System.out.println("Expected: " + OUTPUTS[i]);
        System.out.println("Got: " + result);
        return;
      }
    }

    System.out.println("ORIGINAL tests passed.");

    for (var i = 0; i < INPUTS.length; i++) {
      final var result = revision(INPUTS[i]);

      if (!result.equals(OUTPUTS[i])) {
        System.out.println("REVISION test failed!");
        System.out.println("Input: " + INPUTS[i]);
        System.out.println("Expected: " + OUTPUTS[i]);
        System.out.println("Got: " + result);
        return;
      }
    }

    System.out.println("ALL tests passed.");
  }

  private static String regex(final String text) {
    return text.trim().replaceAll("\\s*\n\\s*", " ");
  }

  private static String revision(final String text) {
    final var result = new StringBuilder(text.length());
    boolean wasSpace = false;

    for (final var c : text.toCharArray()) {
      final var isSpace = Character.isWhitespace(c);
      final var toAppend = isSpace ? ' ' : c;

      if (!wasSpace || !isSpace) {
        result.append(toAppend);
      }

      wasSpace = isSpace;
    }

    return result.toString().trim();
  }
}

I've simplified the algorithm and showed the edge case that fails.

The procedural implementation is not the same as the regular expression.

Notice that instantiating the StringBuilder with text.length() will have modest performance improvements for large strings.

Have you tried pre-compiling the regex as a Pattern constant, instead, to see if there are any performance gains?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants