use java string switch for findPrototypeId

[rbri]

Have updated SwitchGenerator and regenerate all classes based on @tuchida idea (#847)

[tuchida]

Thanks!
Do you have any data on how much faster ./gradlew testBenchmark and Test262SuiteTest are?

[rbri]

There is no notable performance change for Test262SuiteTest.

[tuchida]

I benchmarked ./gradlew testBenchmark on the master and string_switch branches.
https://docs.google.com/spreadsheets/d/1SxhmV0IQ9P534BAVdaf8SDwCyH08iA6L1MfT_slkZ8A/edit?usp=sharing

V8Benchmark.cryptoDecrypt seems to be faster, while SunSpiderBenchmark.accessBinaryTrees and SunSpiderBenchmark.threeDMorph are slower.

[tuchida]

NativeMath#findPrototypeId seems to have slowed down.

rhino/src/org/mozilla/javascript/NativeMath.java

Line 465 in 58f7c7c

protected int findPrototypeId(String s)

Math.PI, etc. are static properties, so it looks like they should be initialized with fillConstructorProperties, but why is the code written in such a way that it is set to prototype?

[gbrail]

As I recall that's all historical stuff and probably makes no sense for backward compatibility. I'm having a super-busy week but I hope to be able to think clearly about all this stuff soon. Here's one question: Do we think that string switch in Java is implemented any more efficiently than a series of if/then statements? If not then this would at least be a theoretical performance regression. In the past I've also tried to see if there are "perfect hashing" libraries for Java that would be a cleaner way to solve this problem but I never found one that was really usable -- but that's exactly what we're trying to do here.

…

On Wed, Mar 24, 2021 at 8:20 AM tuchida ***@***.***> wrote: NativeMath#findPrototypeId seems to have slowed down. https://github.com/mozilla/rhino/blob/58f7c7c7f3cbc0228055b27e5c45348dca4e8854/src/org/mozilla/javascript/NativeMath.java#L465 Math.PI, etc. are static properties, so it looks like they should be initialized with fillConstructorProperties, but why is the code written in such a way that it is set to prototype? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#848 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I2ZXZA47YDTFQZK7W3TTFH7KTANCNFSM4ZRL75WQ> .

[tuchida]

It seems to make a lookupswitch using hashCode.
https://stackoverflow.com/questions/22110707/how-is-string-in-switch-statement-more-efficient-than-corresponding-if-else-stat

This is a javap check of NativeMath#findPrototypeId. (I compiled it with Java8.)

// $ javap -cp buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar -p -v org.mozilla.javascript.NativeMath

  protected int findPrototypeId(java.lang.String);
    descriptor: (Ljava/lang/String;)I
    flags: ACC_PROTECTED
    Code:
      stack=3, locals=5, args_size=2
         0: getstatic     #12                 // Field java/lang/System.out:Ljava/io/PrintStream;
         3: new           #13                 // class java/lang/StringBuilder
         6: dup
         7: invokespecial #14                 // Method java/lang/StringBuilder."<init>":()V
        10: ldc           #147                // String findPrototypeId:
        12: invokevirtual #16                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
        15: aload_1
        16: invokevirtual #16                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
        19: invokevirtual #18                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
        22: invokevirtual #19                 // Method java/io/PrintStream.println:(Ljava/lang/String;)V
        25: aload_1
        26: astore_3
        27: iconst_m1
        28: istore        4
        30: aload_3
        31: invokevirtual #148                // Method java/lang/String.hashCode:()I
        34: lookupswitch  { // 44
             -2044029854: 1046
             -1781441930: 396
             -1266089752: 934
             -1249363324: 1062
              -938285885: 614
                      69: 966
                    2553: 982
                   75504: 1014
                   96370: 411
                   98695: 502
                  100893: 518
                  107332: 550
                  107876: 566
                  108114: 582
                  111192: 598
                  113880: 646
                  114593: 678
                 2340641: 998
                 2988422: 426
                 3003607: 441
                 3004320: 456
                 3047137: 694
                 3049733: 486
                 3059649: 710
                 3236539: 822
                 3327342: 918
                 3530173: 902
                 3530384: 790
                 3538208: 662
                 3552487: 806
                72610935: 1030
                79146770: 1078
                92641186: 854
                93111921: 870
                93133970: 471
                93134024: 886
                94764880: 950
                96961601: 726
                97526796: 534
                99762084: 742
               103147619: 774
               103147683: 758
               108704142: 630
               110640556: 838
                 default: 1091
            }
       396: aload_3
       397: ldc           #20                 // String toSource
       399: invokevirtual #149                // Method java/lang/String.equals:(Ljava/lang/Object;)Z
       402: ifeq          1091
       405: iconst_0
       406: istore        4
       408: goto          1091
       411: aload_3
       412: ldc           #21                 // String abs
       414: invokevirtual #149                // Method java/lang/String.equals:(Ljava/lang/Object;)Z
       417: ifeq          1091
       420: iconst_1
       421: istore        4
       423: goto          1091
       426: aload_3

[rbri]

Have applied this patch to the rhino fork used by HtmlUnit. The test suite does not show any notable changes regarding performance. Still like to see this merged because the code is much cleaner and maybe we will benefit from JDK optimizations in the future.

[tuchida]

https://github.com/mozilla/rhino/blob/58f7c7c7f3cbc0228055b27e5c45348dca4e8854/toolsrc/org/mozilla/javascript/tools/idswitch/Main.java

Will you continue to use this?
Since the code is now simple, I don't see any problem in writing it manually without generating the code.

[gbrail]

Yes, I agree that we should make this change, although it is just short of it's 20th birthday ;-)

I see that @rbri did this by changing the "idswitch" tool. I think this is actually a good idea as it automated the conversion. We can leave it in for now -- over time as people want to just do things manually they can leave out the special "#string_id_map#" comments. We should, however, update the README for "idswitch" to indicate that it's just generating a string switch and is probably not useful once this migration is complete.

[gbrail]

So given that we used the tool, I'm going to merge it now and we can do additional stuff later.