Closed Bug 607711 Opened 14 years ago Closed 14 years ago

JM does not constant-fold JSOP_MOD


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jandem, Assigned: jandem)


(Whiteboard: fixed-in-tracemonkey)


(1 file, 2 obsolete files)

I'll attach a patch for this.
Attached patch Patch (obsolete) — Splinter Review
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);
JM gets these times:
before: 630 ms
after: 450 ms

If I replace "z = x % y" with "z = 1" it's still 450 ms.
Attachment #486415 - Flags: review?(dvander)
Attached patch Patch (obsolete) — Splinter Review
Argh, apparently "hg export -o" appends to the file instead of replacing it.
Attachment #486415 - Attachment is obsolete: true
Attachment #486418 - Flags: review?(dvander)
Attachment #486415 - Flags: review?(dvander)
Comment on attachment 486418 [details] [diff] [review]

>+        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.
Attachment #486418 - Flags: review?(dvander) → review+
And the testcase added!
Attached patch Patch v2Splinter Review
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.
Attachment #486418 - Attachment is obsolete: true
Attachment #486573 - Flags: review?(dvander)
Attachment #486573 - Flags: review?(dvander) → review+
Anyone willing to land this? I don't want to whine, but I'm afraid this will bitrot...

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.
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: fixed-in-tracemonkey
I wouldn't mind that. Sorry for letting this slip off my radar.
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.