Bug 152419 – Add BinarySnippetTester for verifying binary snippet fast path results.

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Mark Lam]

This is a testing infrastructure that uses MASM probes to automatically compare the snippet fast path results with an equivalent evaluation performed using a C++ function.

[Mark Lam]

Created attachment 267613 [details]
proposed patch.

[WebKit Commit Bot]

Attachment 267613 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/BinarySnippetTester.h:56:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/BinarySnippetTester.h:57:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/BinarySnippetTester.h:58:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 267614 [details]
proposed patch with typo fix.

[WebKit Commit Bot]

Attachment 267614 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/BinarySnippetTester.h:56:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/BinarySnippetTester.h:57:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/BinarySnippetTester.h:58:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 267614 [details]
proposed patch with typo fix.

I really don't like fiddly testing mechanisms like this, which require significant low-level coding, and intrusive adoption inside the tested code.

If we did all our testing like this, the code base would be completely unintelligible. Also, since the test code is equally low-level to the code being tested, any failure has equal probability to be a failure in the test code or a failure in the tested code. Also, since the test code is embedded in the code being tested, if we change the engine significantly, we may lose test coverage (whereas a test written in client code or in JavaScript can last basically forever).

[Mark Lam]

(In reply to comment #5)
> Comment on attachment 267614 [details]
> proposed patch with typo fix.
> 
> I really don't like fiddly testing mechanisms like this, which require
> significant low-level coding, and intrusive adoption inside the tested code.
> 
> If we did all our testing like this, the code base would be completely
> unintelligible. Also, since the test code is equally low-level to the code
> being tested, any failure has equal probability to be a failure in the test
> code or a failure in the tested code. Also, since the test code is embedded
> in the code being tested, if we change the engine significantly, we may lose
> test coverage (whereas a test written in client code or in JavaScript can
> last basically forever).

This is the trade off between white box and black box testing.  The tests that you favor are black box tests and do indeed have the advantages that you speak of.  And yes, the disadvantages you spoke of for the test mechanism in this patch are the disadvantages of white box tests (which are necessarily closely coupled with the code being tested).

The advantages of white box tests though are that they fail fast at the direct point of failure.  That's why they proved useful to me when I was debugging failures that manifested while running benchmarks (or other similarly large workloads) but that did not manifest when running our test suites.  This targeted test mechanism is more powerful as a debugging aid than the black box tests.  And yes, I did enhance our (black box) test suite coverage with the missing cases thereafter. 

I think that both types of tests can be valuable.  But if you think this patch pollutes the readability of the code too much, I can stick with manually applying this patch every time I need this functionality.

[Mark Lam]

We're not doing this.