Use JS String includes/startsWith/endsWith

[niloc132]

Even when completely inlined, should produce smaller code than the previous emulation, and depending on the browser implementation this could perform slightly better as well.

[niloc132]

Let's wait and merge after #10106, just in case there's a merge issue so it goes in this PR.

[vjay82]

Maybe you like to replace String.replace (which is petty heavyweight) with JS String.replaceAll too?

[niloc132]

Maybe you like to replace String.replace (which is petty heavyweight) with JS String.replaceAll too?

Can you elaborate? GWT's String.replace already calls String.replaceAll, with an extra ,'g', which calls JS String.replace? At least from mdn, the difference between replace() and replaceAll() in the browser is that ,'g' flag, so it would be just saving a byte?

On the other hand, Java String.replaceAll takes a string as regex (as both JS String.replace and String.replaceAll do), but String.replace takes a char or string, and doesn't treat the first arg as a regex.

The heavyweight parts of GWT's String.replace is making the passed string literal into a regexp (plus then calling GWT's own String.replaceAll adds backref replacements).

I agree there is probably some savings to be had here, but it doesn't look substantial, and there is no direct analog like there is for these three methods?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace

[vjay82]

I'm pretty confident that

public String replace(CharSequence from, CharSequence to) {
  return asNativeString().replaceAll(from.toString(), to.toString());
}

beats

public String replace(CharSequence from, CharSequence to) {
  // Implementation note: This uses a regex replacement instead of
  // a string literal replacement because Safari does not
  // follow the spec for "$$" in the replacement string: it
  // will insert a literal "$$". IE and Firefox, meanwhile,
  // treat "$$" as "$".

  // Escape regex special characters from literal replacement string.
  String regex = from.toString().replaceAll("([/\\\\\\.\\*\\+\\?\\|\\(\\)\\[\\]\\{\\}$^])", "\\\\$1");
  // Escape $ since it is for match backrefs and \ since it is used to escape
  // $.
  String replacement = to.toString().replaceAll("\\\\", "\\\\\\\\").replaceAll("\\$", "\\\\$");

  return replaceAll(regex, replacement); 
}

with additional cost of

/**
   * This method converts Java-escaped dollar signs "\$" into JavaScript-escaped
   * dollar signs "$$", and removes all other lone backslashes, which serve as
   * escapes in Java but are passed through literally in JavaScript.
   *
   * @skip
   */
  private static String translateReplaceString(String replaceStr) {
    int pos = 0;
    while (0 <= (pos = replaceStr.indexOf("\\", pos))) {
      if (replaceStr.charAt(pos + 1) == '$') {
        replaceStr = replaceStr.substring(0, pos) + "$"
            + replaceStr.substring(++pos);
      } else {
        replaceStr = replaceStr.substring(0, pos) + replaceStr.substring(++pos);
      }
    }
    return replaceStr;
  }

in complexity, size and performance (not measured).

[niloc132]

Thanks, it looks like I misread the definition, as pattern can be a String... but then it won't be a pattern, but just a literal to find. Contrast with String.match()/matchAll() which will interpret its pattern as a regexp even if it is a String.

I'll take a quick look, if its that easy, its definitely worth it.

[zbynek]

Simplifying replace(CharSequence, CharSequence) is a good idea, the same applies to replace(char, char).

[niloc132]

replace(char, char) does get slightly messy (inlining with constant chars doesn't often end happily today, I have a followup to help with that), but definitely worth a look too, yes.

[niloc132]

@zbynek aside: it looks like adding any label to a PR triggers a build now if ready is present?

[vjay82]

I'm using this personally, while also transforming all constant chars to strings by a preprocessor (until your cleaner solution is available). I think that should inline quite ok.

public String replace(char from, char to) {
  return asNativeString().replaceAll(NativeString.fromCharCode(from), NativeString.fromCharCode(to));
}

[niloc132]

Consider String.valueOf rather than directly using NativeString.fromCharCode - the followup I'm working on deals with the fact that if you have str.replace('a', 'b'), the NativeString.fromCharCode will get inlined instead of rewriting char 'a' (and b) into "a", etc. The DeadCodeElimination pass is supposed to do this, but depending on where the char literal comes from, it might miss out on the chance - but implemented as you have it, it never gets the chance, and you'll always get something like

a = b.replaceAll(String.fromCharCode(97), String.fromCharCode(98))

[niloc132]

Digging in more deeply, the proposed replacement is not enough, replaceAll(String, String) in JS doesn't quite behave the same as replace(String, String), though it looks like the old replacement regex will suffice here.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#specifying_a_string_as_the_replacement

Example in Java (caught by GWT's unit tests):

jshell> "a[x]b".replace("[x]", "$$")
$1 ==> "a$$b"

Whereas in Javascript (tested chrome, ff, safari):

> "a[x]b".replaceAll("[x]", "$$")
< "a$b" 

Failing unit test:

Testcase: testReplaceString took 0.001 sec
	FAILED
expected: <a$$b>, actual: <a$b>
junit.framework.AssertionFailedError: expected: <a$$b>, actual: <a$b>
	at java.lang.Throwable.Throwable(com/google/gwt/emul/java/lang/Throwable.java:73)
	at java.lang.Error.Error(com/google/gwt/emul/java/lang/Error.java:30)
	at java.lang.AssertionError.AssertionError(com/google/gwt/emul/java/lang/AssertionError.java:51)
	at junit.framework.AssertionFailedError.AssertionFailedError(com/google/gwt/junit/translatable/junit/framework/AssertionFailedError.java:27)
	at com.google.gwt.emultest.java.lang.StringTest.testReplaceString(com/google/gwt/emultest/java/lang/StringTest.java:757)

Additionally, some further testing suggests that Chrome doesn't behave the way it used to when it comes to escaped slashes, and our unit tests fail when run in a browser - so even what we have today is wrong, and needs more vetting.

...So since we need at least part of the old function and the plain JS call isn't enough, I'm going to leave out replace/replaceAll from this patch for them to be addressed later, and in a more concrete way. Check your own impl @vjay82 - maybe you never use a replacement $, but I bet someone somewhere does, and wouldn't want them to be surprised by this.

[vjay82]

For the time being I will probably go with this as a quick fix:

	public String replace(CharSequence from, CharSequence to) {
		return asNativeString().replaceAll(from.toString(), () -> {
			return to.toString();
		});
	}

Using a function is likely not cheap but then relying on a regex and/or lots of transformations is probably neither.
More research is needed.
Thank you for the info.