1467907 - Create a CacheIR IC for Compare(String,Int32)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

In Bug 1341261 I have been working on replacing the Shared IC with CacheIR. Using the CACHEIR_LOGS, it's become pretty obvious we have a coverage gap for comparing strings to integers.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

(Just a thought that this should be reasonably simple for indexed strings, assuming hasIndexValue and getIndexValue do what they should)

Comment 2

[Ted Campbell [:tcampbell]]

We apparently have a dtoa cache. I wonder if an IC for string identity to specific integer would ever be useful.
https://searchfox.org/mozilla-central/source/js/src/vm/Realm.h#410

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Add an IC for String Int32 comparison

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Add an IC for String Int32 comparison

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 9002448 [details] [diff] [review]
Add an IC for String Int32 comparison

Review of attachment 9002448 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, but some suggestions below.

::: js/src/jit/CacheIR.cpp
@@ +5012,5 @@
> +CompareIRGenerator::tryAttachStringIndexed(ValOperandId lhsId, ValOperandId rhsId)
> +{
> +    if (!(lhsVal_.isString() && rhsVal_.isInt32()) &&
> +        !(rhsVal_.isString() && lhsVal_.isInt32()))
> +        return false;

Nit: add {}, with { on its own line.

@@ +5019,5 @@
> +        return false;
> +
> +    JSString* str = lhsVal_.isString() ? lhsVal_.toString() : rhsVal_.toString();
> +    // Flat strings don't have indexes
> +    if (!str->isFlat())

The comment doesn't match the code.

@@ +5023,5 @@
> +    if (!str->isFlat())
> +        return false;
> +
> +    // Don't try to attach if the string has no index value.
> +    if (!str->hasIndexValue())

GuardAndGetIndexFromString will call GetIndexFromString if the string has no index value, so should we call GetIndexFromString instead here? What's perf like when comparing a string-without-index to an int32 if we do that?

I'm also a bit worried about the stub failing for negative integers for no good reason (-1 is pretty common). What do you think about adding guardAndGetInt32FromString? It could work mostly the same way, but it would call js::StringToNumber and fail if the double does not fit in an int32. Or we could use js::StringToNumber and make the stub work for all doubles, that might be even nicer.

@@ +5029,5 @@
> +
> +    auto createGuards = [&](HandleValue v, ValOperandId vId) {
> +        if (v.isString()) {
> +            StringOperandId strId = writer.guardIsString(vId);
> +            return  writer.guardAndGetIndexFromString(strId);

Nit: remove extra space after return.

@@ +5065,5 @@
>      ValOperandId rhsId(writer.setInputOperandId(rhsIndex));
>  
>      // For sloppy equality ops, there are cases this IC does not handle:
>      // - {Symbol} x {Null, Undefined, String, Bool, Number}.
> +    // - {String} x {Null, Undefined, Symbol, Bool, Number}.

Unrelated, but is this true, we don't have an IC for |<anything> ==/!= {null,undefined,true,false}|? Maybe worth fixing too.

@@ +5108,5 @@
>          return true;
>      if (tryAttachNumber(lhsId, rhsId))
>          return true;
>  
> +    if (tryAttachStringIndexed(lhsId,rhsId))

Nit: add a space after the ','

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1467907 - Add an IC for String x Number comparison r=jandem

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: compare_bench.js

Benchmarking shows the patch to be ~2x faster on a concentrated microbenchmark for the cases where it applies. (x64, opt shell build on OS/X run with no arguments. 

Note, The inputs of this benchmark are absolutely worth questioning (2 indexed strings, 3-non indexed), I think this is overall a minor issue.

Without: 

    String_Number_GT1       1.404970947265625
    String_Number_GTE1      1.399468994140625
    String_Number_LT1       1.378340087890625
    String_Number_LTE1      1.40908203125
    String_Number_EQ1       1.719333984375
    String_Number_NEQ1      1.7414580078125
    String_Number_SEQ1      0.47639501953125
    String_Number_SNEQ1     0.47108203125

With: 

    String_Number_GT1       0.724429931640625
    String_Number_GTE1      0.742053955078125
    String_Number_LT1       0.7125439453125
    String_Number_LTE1      0.715946044921875
    String_Number_EQ1       0.825533935546875
    String_Number_NEQ1      0.83451416015625
    String_Number_SEQ1      0.47579296875
    String_Number_SNEQ1     0.48748681640625

The lack of impact for SEQ and SNEQ are because those cases are being handled by a separate IC (the strictly different types IC).

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9003888 [details]
Bug 1467907 - Add an IC for String x Number comparison r=jandem

Jan de Mooij [:jandem] has approved the revision.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a23a0c5054d9
Add an IC for String x Number comparison r=jandem

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/a23a0c5054d9

Comment 11

[Jens Hausdorf :jens1o [ni? me]]

Shouldn't this bug have a `perf` keyword?

Comment 12

[Matthew Gaudet (he/him) [:mgaudet]]

I mean, perhaps? I guess my hesitation from marking this as 'perf' mostly relates to us not having a specific use case in mind, outside of us trying to avoid performance cliffs. 

Is there a particular purpose you'd want this marked perf for?

Comment 13

[Jens Hausdorf :jens1o [ni? me]]

Just to make it easier for anybody to find all performance improvements.