1490441 - Consider IC for add/sub on Date

Status: RESOLVED FIXED

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

Description

[Tom S. (please needinfo tschuster)]

Running Octane in the shell causes almost 50k BinaryArith failures for a JSOP_SUB where lhs and rhs are Date objects.

{
  "name":"BinaryArith",
  "file":"/home/tom/projects/inbound/js/src/octane/base.js",
  "mode":0,
  "line":308,
  "column":16,
  "pc":"7f42b68cb41e",
  "op":"sub",
  "rhs":{
    "type":"Date",
    "value":"7f42bada46c0 (shape: 7f42b428e628)"
  },
  "lhs":{
    "type":"Date",
    "value":"7f42bada9b80 (shape: 7f42b428e628)"
  }
},

Comment 1

[Jan de Mooij [:jandem]]

It might make sense to first add an |Object <op> Object| stub to handle these cases with a VM call, before specializing Date :)

Comment 2

[Debadree Chatterjee]

In case this bug is still relevant would adding a specialisation of Object <op> Object specialisation over here: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIRGenerator.h#895 with specialisation say tryAttachObjectObjectArith which would in turn call these methods via callVM: https://searchfox.org/mozilla-central/source/js/src/vm/Interpreter-inl.h#624,693,725,741,765 be the way to approach this issue?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Debadree Chatterjee from comment #2)

In case this bug is still relevant would adding a specialisation of Object <op> Object specialisation over here: https://searchfox.org/mozilla-central/source/js/src/jit/CacheIRGenerator.h#895 with specialisation say tryAttachObjectObjectArith which would in turn call these methods via callVM: https://searchfox.org/mozilla-central/source/js/src/vm/Interpreter-inl.h#624,693,725,741,765 be the way to approach this issue?

Exactly. We could call js::AddValues, js::SubValues etc directly from JIT code if the operands are both objects.

I'm not sure how much of a performance improvement it would be over the IC fallback code (IonBinaryArithIC::update and the Baseline version, DoBinaryArithFallback) because that does roughly the same thing although it also does some other things.

As a first step, I'd suggest adding some logging to IonBinaryArithIC::update and DoBinaryArithFallback to determine for the different JSOps handled there how common it is to have two object values as operands (could be just a printf). You could then run the Speedometer 3 and JetStream 2 benchmarks in the browser and analyze the results.

If you do this we should also look at the type of objects (from obj->getClass()->name). If 99% of the objects are Date objects, maybe it makes sense to do something more efficient for just Date objects.

Comment 4

[Debadree Chatterjee]

Indeed it seems like Date is somewhat frequently used in speedometer! the only case seems to be Date - Date or Date - number (which makes sense i guess the use case is calculating time diff?) anyway running speedometer on the browser and sampling what hits IonBinaryArithIC::update and DoBinaryArithFallback i get the following https://pastebin.mozilla.org/TzxmEdt2 ! I am now investigating whether adding a specialisation improves it how much!

Comment 5

[Debadree Chatterjee]

Attachment: Bug 1490441 - Part 1: Add fast path for date objects in OrdinaryToPrimitive. r=#spidermonkey-reviewers!

Comment 6

[Debadree Chatterjee]

Attachment: Bug 1490441 - Part 2: Add CacheIR specialisation for Date ops. r=#spidermonkey-reviewers!

Depends on D223007

Comment 7

[Steve Fink [:sfink] [:s:]]

Assuming the "Op is Add|Sub" output comes after the lhs and rhs, that comes to:

Count	lhs	Op	rhs
1	string	Add	BoundFunctionObject
8	Date	Sub	Date
10	string	Add	Object
20	Object	Add	string
22	number	Sub	Date
350	Function	Sub	number
5092	Date	Sub	number

Why is it doing so many Function - number calls? Did I get the format wrong?

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Results from pastebin, in the form of the perl script summarizing them

Comment 9

[Debadree Chatterjee]

Thank you so much for summarising this is so nice!!

Yeah that seemed weird Function - number i thought about investigating it maybe its possible to print out the code that does this using the JSScript* structure? but i got to excited to write code instead hehe

Comment 10

[Debadree Chatterjee]

Some more analysis using the amazing perl script provided by :sfink!

(note that we only log if either of lhs or rhs is an object)

On nightly startup and loading the homepage

Count	lhs	Op	rhs
1	string	Add	BoundFunctionObject
6	Date	Sub	number
379	Date	Sub	Date

On Jetstream2

Count	lhs	Op	rhs
1	string	Add	BoundFunctionObject
1	string	Add	Date
1	Date	Add	string
2	string	Add	RegExp
3	Date	Sub	Date
10	Object	Add	string
11	number	Sub	Date
45	Function	Add	string
84	string	Add	Object
101	string	Add	Array
1763	string	Add	String

On Octane2

Count	lhs	Op	rhs
19301	Date	Sub	Date

Regarding the weird Function <op> number operations:
I found this function seems to be the culprit https://github.com/WebKit/Speedometer/blob/ea14d27e03603bf796447598cf23b83788fca3f7/resources/perf.webkit.org/public/v3/components/time-series-chart.js#L414

but exact line number unable to figure out :-(

I used the following code the figure out the code i couldnt find a way to directly dump the code tho:

diff --git a/js/src/jit/BaselineIC.cpp b/js/src/jit/BaselineIC.cpp
--- a/js/src/jit/BaselineIC.cpp
+++ b/js/src/jit/BaselineIC.cpp
@@ -2276,6 +2276,36 @@ bool DoBinaryArithFallback(JSContext* cx
   // generating stubs.
   RootedValue lhsCopy(cx, lhs);
   RootedValue rhsCopy(cx, rhs);
+  if (lhsCopy.isObject() || rhsCopy.isObject()) {
+    printf("===========DoBinaryArithFallback===========\n");
+    printf("lhs is of type %s and class %s\n", InformalValueTypeName(lhsCopy),
+           lhsCopy.isObject() ? lhsCopy.toObject().getClass()->name : "N/A");
+    printf("rhs is of type %s and class %s\n", InformalValueTypeName(rhsCopy),
+           rhsCopy.isObject() ? rhsCopy.toObject().getClass()->name : "N/A");
+    if (lhsCopy.isObject() && strcmp(lhsCopy.toObject().getClass()->name, "Function") == 0) {
+      frame->script()->dump(cx);
+      printf("the line num: %d", PCToLineNumber(frame->script(), pc));
+    }
+    if (rhsCopy.isObject() && strcmp(rhsCopy.toObject().getClass()->name, "Function") == 0) {
+      frame->script()->dump(cx);
+      printf("the line num: %d", PCToLineNumber(frame->script(), pc));
+    }
+    printf("===========================================\n");
+    switch (op) {
+      case JSOp::Add:
+        printf("Op is Add\n");
+        break;
+      case JSOp::Sub:
+        printf("Op is Sub\n");
+        break;
+      case JSOp::Mul:
+        printf("Op is Mul\n");
+        break;
+      case JSOp::Div:
+        printf("Op is Div\n");
+        break;
+    }
+  }
 
   // Perform the arith operation.
   switch (op) {
diff --git a/js/src/jit/IonIC.cpp b/js/src/jit/IonIC.cpp
--- a/js/src/jit/IonIC.cpp
+++ b/js/src/jit/IonIC.cpp
@@ -589,6 +589,36 @@ bool IonBinaryArithIC::update(JSContext*
   // generating stubs.
   RootedValue lhsCopy(cx, lhs);
   RootedValue rhsCopy(cx, rhs);
+  if (lhsCopy.isObject() || rhsCopy.isObject()) {
+    printf("===========IonBinaryArithIC===========\n");
+    printf("lhs is of type %s and class %s\n", InformalValueTypeName(lhsCopy),
+           lhsCopy.isObject() ? lhsCopy.toObject().getClass()->name : "N/A");
+    printf("rhs is of type %s and class %s\n", InformalValueTypeName(rhsCopy),
+           rhsCopy.isObject() ? rhsCopy.toObject().getClass()->name : "N/A");
+    printf("======================================\n");
+    if (lhsCopy.isObject() && strcmp(lhsCopy.toObject().getClass()->name, "Function") == 0) {
+      ic->script()->dump(cx);
+      printf("the line num: %d", PCToLineNumber(ic->script(), pc));
+    }
+    if (rhsCopy.isObject() && strcmp(rhsCopy.toObject().getClass()->name, "Function") == 0) {
+      ic->script()->dump(cx);
+      printf("the line num: %d", PCToLineNumber(ic->script(), pc));
+    }
+    switch (op) {
+      case JSOp::Add:
+        printf("Op is Add\n");
+        break;
+      case JSOp::Sub:
+        printf("Op is Sub\n");
+        break;
+      case JSOp::Mul:
+        printf("Op is Mul\n");
+        break;
+      case JSOp::Div:
+        printf("Op is Div\n");
+        break;
+    }
+  }
 
   // Perform the compare operation.
   switch (op) {

Are there any other benchmarks i could investigate in?

Also thank you once again :sfink for this amazing perl script i am inclined to try learning perl now!

Thank you!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94b2517be614
Part 1: Add fast path for date objects in OrdinaryToPrimitive. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/4a150e5d32d1
Part 2: Add CacheIR specialisation for Date ops. r=jandem,anba

Comment 12

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/94b2517be614
https://hg.mozilla.org/mozilla-central/rev/4a150e5d32d1