1318285 - Add a testing mechanism for CacheIR

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Jan de Mooij [:jandem]]

It would be really nice to have correctness tests for ICs, especially for the more exotic ones (DOM proxies for instance), to ensure we don't regress IC coverage.

At some point we should add some testing functions to expose the CacheIR we emit, and then tests can query this. Something like:

  domProxy.someGetter;
  assertEq(getCacheIR("GetProp"), "GuardShape 0; CallDOMProxy 0;");

Comment 1

[Tom S [:evilpie]]

This would be really nice! Adding at least one test per IC would be a very good idea. Boris asked for something like this in bug 1319087 for CCW tests.

Comment 2

[Jan de Mooij [:jandem]]

It might be easier for now to use the trackAttached mechanism you added for CacheIR analyzer, because we don't have an easy way atm to iterate CacheIR instructions (and skipping the operands). That could be fixed, of course.

Comment 3

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

I've been thinking about this issue from a slightly different perspective than the IR directly, more in terms of functional testing. 

One of the challenges when working with inline caches is the possibility for the cache to fail silently, taking the failure path unexpectedly. The nature of the inline cache chains is such that this can lead to an invisible performance problem. Using CacheIR, this can be caught by inspecting the CacheIR logs, but we don't have automated testing to catch regressions on this front. 

In order to write test cases, one of the most important things that can be done is to start providing introspection capabilities to test cases, such that they are able to interrogate what has happened in the inline caches. What I'm proposing here is the application of a more general technique, but I wanted some feedback here where it tied directly to a real problem. 

Proposal: Annotate inline caches with counter increments, where the counter values can be interrogated by a test case. 

More concrete: 

Suppose we did something like this (note, doesn't compile, lots of issues -- for elucidation only): 

>     diff --git a/js/src/jit/CacheIR.h b/js/src/jit/CacheIR.h
>     --- a/js/src/jit/CacheIR.h
>     +++ b/js/src/jit/CacheIR.h
>     @@ -1045,6 +1045,11 @@ class MOZ_RAII CacheIRWriter : public JS
>          }
>          void returnFromIC() {
>              writeOp(CacheOp::ReturnFromIC);
>     +        writePointer(nullptr);
>     +    }
>     +    void returnFromIC(const char* str) {
>     +        writeOp(CacheOp::ReturnFromIC);
>     +        writePointer(const_cast<char*>(str));
>          }
>          void wrapResult() {
>              writeOp(CacheOp::WrapResult);
>     diff --git a/js/src/jit/BaselineCacheIRCompiler.cpp b/js/src/jit/BaselineCacheIRCompiler.cpp
>     --- a/js/src/jit/BaselineCacheIRCompiler.cpp
>     +++ b/js/src/jit/BaselineCacheIRCompiler.cpp
>     @@ -1947,6 +1947,10 @@ bool
>      BaselineCacheIRCompiler::emitReturnFromIC()
>      {
>          allocator.discardStack(masm);
>     +    const char* str = reinterpret_cast<char*>(reader.pointer());
>     +    if (str) {
>     +        masm.incrementInt32Value(AddressImmediate(LookupStorage(str));
>     +    }
>          EmitReturnFromIC(masm);
>          return true;
>      }
>     diff --git a/js/src/jit/CacheIR.cpp b/js/src/jit/CacheIR.cpp
>     --- a/js/src/jit/CacheIR.cpp
>     +++ b/js/src/jit/CacheIR.cpp
>     @@ -4616,7 +4616,7 @@ CompareIRGenerator::tryAttachSymbol(ValO
>          SymbolOperandId lhsSymId = writer.guardIsSymbol(lhsId);
>          SymbolOperandId rhsSymId = writer.guardIsSymbol(rhsId);
>          writer.compareSymbolResult(op_, lhsSymId, rhsSymId);
>     -    writer.returnFromIC();
>     +    writer.returnFromIC("Compare:Symbol");
>      
>          trackAttached("Symbol");
>          return true;



and we added the JS Shell TestFunctions to allow looking up the same storage value as the string: 


>     js> SomeComparisonSymbolTestCase(); 
>     js> assertEq(getICHits("Compare:Symbol"), 1); 


Given these two pieces, we're on our way to being able to assert whether or not the IC returned as expected. With a little more effort, I suspect we could inject failure counters as well. This all could happen under a debug option. 

Issues/Discussion: 

* The above is a simplification, because we'd likely want to associate the string with some information about the script, like line number, filename, etc. 
  * However, Baseline shares IC, which rather puts a kink in this. There's some possible responses I think though.
* The challenges I don't have answers for quite yet: How do we allocate memory storage for the counter bits such that it has the correct lifetime -- ideally it'd be the same lifetime as the JIT code, and the counter values would be rolled up during JSScript::finalize(). 
* The mechanism of having 'named' counter locations, and the ability to query / dump them can be very very useful. This proposal is heavily influenced by my experience with the OMR compiler technology that had a fairly complete story for counters (https://github.com/eclipse/omr/blob/master/compiler/ras/DebugCounter.md). It might be worth fleshing out a story similar to that after getting some experience with how to do it right for the ICs. 
* There's also the obvious extension here of trying to export counter knowledge into the Gecko Profiler, in such a way that we could discover quickly sub-optimal behaviour; we could even put counters on slow-path hits, expanding usefulness beyond just ICs (thanks sfink). 
* Writing test cases in a way that's independent of baseline/ion thresholds is definitely a piece of art, and won't be trivial.

Comment 4

[Jeff Walden [:Waldo]]

Using numbers for counters has been done before for the ancient trap() function in the JS shell -- to set breakpoints on specific bytecode offsets and do/test stuff with them.  Every test that used such was super-fragile.  I have zero doubt that some people mis-updated tests when bytecode changes occurred, in a way that the test didn't test what it meant to, then far down the line the test had lost all connection to the reality it meant to introspect.

I personally would consider it an absolute blocker if cache-locations could not be precisely identified in the source text and given names that would never vary without their code being rewritten.

Comment 5

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

I definitely get the objection. I'm not knowledgeable enough to suggest a good alternative right now that would be so specific one could name an IC.

I think the best thing I can think of would be to not expose lines, but to instead expose the state of the ICs to the test case and let the test case infer certain facts: 

> js> x = getICHits("Compare:Symbol"); 
> js> SomeComparisonSymbolTestCase(); 
> js> assertEq(getICHits("Compare:Symbol"), x + 1); 

There's some sugar which could make that nicer, but nice feature of this is that the test cases can be refactored with no more difficulty than any other stateful test case.