Closed Bug 1466189 Opened 2 years ago Closed 2 years ago

Equal length strings need IC coverage


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




Tracking Status
firefox62 --- fixed


(Reporter: mgaudet, Assigned: mgaudet)


(Blocks 1 open bug)



(3 files)

Running the attached test case with CACHEIR_LOGS=1 there are thousands of entries for string equality checks. 

There's a couple of reasons: With CacheIR, the CacheIR logs indicate when the IR generator successfully generates IR. 

We end up generating IR many many times for this case however, because the Compare IC uses MacroAssembler::compareStrings [1] which is a fallible operation: It can only do comparisons based on string length or identity, which means that two distinct strings that are of the same length will always fail the IC. 

In Baseline, the reason we don't attach thousands of duplicate stubs is that we are able to detect identical stub content before converting the CacheIR to native code. 

This whole thing seems a little suboptimal though, as it means every IC miss on a string operation generates new IR, even in fatal circumstances. 

I'm not sure if it's a better idea to tackle the fallible stub, or if we need to strengthen the story around CacheIR logging (potentially the answer is both). 

The stub should call some helper function to compare the chars instead of failing..
Also, I'd say the logging is working perfectly fine (at least this part) - this is exactly the kind of silly perf issue we want to catch and fix.
That's a good point. I'm going to re-title this bug and we'll use this to fix the string compare miss.
Blocks: CacheIR
Summary: Equal length strings cause spam to CacheIR logs → Equal length strings need IC coverage
try build:

Surprisingly (to me) I didn't find much in the way of win, even on a microbenchmark.
Attachment #8983842 - Flags: review?(tcampbell)
Assignee: nobody → mgaudet
Comment on attachment 8983842 [details] [diff] [review]
Handle equal length strings in Compare IC

Review of attachment 8983842 [details] [diff] [review]:

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +980,5 @@
>      return true;
>  }
> +typedef bool (*StringCompareFn)(JSContext*, HandleString, HandleString, bool*);
> +static const VMFunction StringsEqualInfo =

Common these up with the copy in CodeGenerator.cpp
Each copy generates a separate trampoline on startup and they aren't the little pointer wrappers that they appear to be.

Although, it looks like we de-dupe the trampolines so this may be fine.

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +1398,5 @@
> +    {
> +        return false;
> +    }
> +
> +    masm.storeCallBoolResult(scratch);

This results in double zero extension since the trampoline already does this in [1]. Can consider using storeCallInt32Result with a comment that the trampoline extends bool to int32.

(I had to look this up myself..)
Attachment #8983842 - Flags: review?(tcampbell) → review+
Pushed by
Handle equal length strings in Compare IC r=tcampbell
Comment on attachment 8983842 [details] [diff] [review]
Handle equal length strings in Compare IC

Review of attachment 8983842 [details] [diff] [review]:

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +1386,5 @@
> +
> +    masm.jump(&done);
> +    masm.bind(&slow);
> +
> +    allocator.discardStack(masm);

Drive-by comment: should this be moved right after addFailurePath? If we take the fast path it's possible the actual machine stack no longer matches the stack tracked in the compiler (because we jump over possible stack manipulation ops). 

Not sure how we can make that more robust..
Comment on attachment 8984181 [details] [diff] [review]
followup patch to make discard stack unconditional

Review of attachment 8984181 [details] [diff] [review]:

Attachment #8984181 - Flags: review?(jdemooij) → review+
Pushed by
followup patch to make discard stack unconditional r=jandem
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.