607711 - JM does not constant-fold JSOP_MOD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

I'll attach a patch for this.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch fixes it. If both sides are int32, we have to perform int modulus, like stubs::Mod. I added a test that fails without it.

For this benchmark:
---
function f() {
    var t0 = new Date;
    for(var i=0; i<100000000; i++) {
	var x = 10, y = 3;
	var z = x % y;
    }
    print(new Date - t0);
}
f();
---
JM gets these times:
before: 630 ms
after: 450 ms

If I replace "z = x % y" with "z = 1" it's still 450 ms.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Argh, apparently "hg export -o" appends to the file instead of replacing it.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 486418 [details] [diff] [review]
Patch

>+        needInt = (L.isInt32() && R.isInt32() && 
>+                   L.toInt32() >= 0 && R.toInt32() >= 0);

This must be R.toInt32() > 0, otherwise:

dvander@dvander:~/mozilla/tracemonkey/js/src$ ./Debug32/js -m
js> function f() { var x = 0; var y = 0; return x % y; }
js> f();
Floating point exception

r=me with that fixed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

And the testcase added!

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Thanks for catching it, dvander. The fix uncovered some (pre-existing) problems with the double case; it was confusing dL and dR. This patch fixes it (looks more like what stubs::Mod does now) and adds tests for these cases.

Comment 6

[Jan de Mooij [:jandem]]

Anyone willing to land this? I don't want to whine, but I'm afraid this will bitrot...

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/137433bc8c2d

I don't mind landing patches (and it's not whining ;-) ).  But the problem for me is that bugs outside my direct view don't get as much attention from me as I'd like (I'm half a year behind on really keeping up with bugmail) -- I only see these if a bugmail skim happens to catch them, which isn't guaranteed.  I think we need something better than just an ad hoc process here.

Would anyone mind if we made it the responsibility of the reviewer to push patches like this, after the author has requested it?  He's the only person already guaranteed to be paying attention to the bug, after all.  And I expect, if we keep relying on me to do this, we're going to keep having multi-week landing delays.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

I wouldn't mind that. Sorry for letting this slip off my radar.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/137433bc8c2d