Math.max with multiple arguments is very slow

RESOLVED DUPLICATE of bug 936234

Status

()

defect
RESOLVED DUPLICATE of bug 936234
7 years ago
3 years ago

People

(Reporter: bzbarsky, Unassigned)

Tracking

Trunk
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

I was just writing a canvas algorithm that wanted to compute the max of the r,g,b values of a pixel.  Writing it like so:

        var gray = Math.max(sourceData[px], sourceData[px+1]);
        gray = Math.max(gray, sourceData[px+2]);

made it run about 2x faster than writing it like so:

        var gray = Math.max(sourceData[px], sourceData[px+1], sourceData[px+2]);

and both times the running time included various other things (putImageData calls and so forth).

I assume we end up with a stubcall instead of inlining, so I'm not sure there's that much that cane be done here... though maybe we should consider inlining 3-arg Math.max.  Maybe.

Chrome has a similar performance cliff, with about the same ratio....
>I assume we end up with a stubcall instead of inlining, so I'm not sure there's that much that >cane be done here... though maybe we should consider inlining 3-arg Math.max.  Maybe.
This is right, we have a optimized version for Math.min/max (int, int), (double/int, double/int).

But in think in practice Math.min, Math.max with 2+ arguments is rather uncommon, so I think we shouldn't bother with JM+TI, also it shouldn't be to hard to do.
(In reply to Tom Schuster [:evilpie] from comment #1)
> But in think in practice Math.min, Math.max with 2+ arguments is rather
> uncommon, so I think we shouldn't bother with JM+TI, also it shouldn't be to
> hard to do.

Can we not do something like:

  if (all arguments are doubles or ints)
    /* Or even always do this at the cost of some codesize, perhaps */
    manually build two-argument max call reduction chain
  else
    leave things alone

or are you saying that this is easy and it's not worth the effort?
To be clear, I'm not too worried about doing this for JM, but I think for Ion we should really think about doing something here if it's not too bad.
Assignee: general → nobody
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 936234
You need to log in before you can comment on or make changes to this bug.