603841 - Kraken: imaging-desaturate starts with array-of-int, ends with array-of-double

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

The main loop of imaging-desaturate.js is this:

  //XXX improve dataset rather than loop
  for (var pixcounter = 0; pixcounter < 200; pixcounter++)
      Pixastic.Actions.desaturate.process(params);

A bigger dataset would indeed be bigger.  But the "repeat 200 times" aspect doesn't work like you might expect -- the image is desaturated, then the desaturated image is desaturated, etc.

The desaturation step looks like this:

  while (p--)
    data[pix-=4] = data[pix1=pix+1] = data[pix2=pix+2] = (data[pix]*0.3 + data[pix1]*0.59 + data[pix2]*0.11);

So every time around the loop, each R/G/B value is getting smaller.  Even stranger, the R/G/B values start of as integers in the range 0..255, but after the first iteration they become doubles.  In other words, the first iteration converts an array-of-int to an array-of-double.  The remaining 199 iterations convert an array-of-double to an array-of-double.  It seems like the desaturate step should really truncate the values.

Also, this characteristic stymies implementations that would like to specialize the array to an array-of-int... you could argue that this is a good test against overspecializing in such a case, but if the benchmark is supposed to be doing something useful, I'd argue against this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  If this is working with image pixels, how come it's using a native JS array instead of CanvasPixelArray or some such?

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> Hmm.  If this is working with image pixels, how come it's using a native JS
> array instead of CanvasPixelArray or some such?

I think a lot of the Kraken benchmarks are very well suited to typed arrays, but Sayre didn't want to use them right now because they aren't widely supported.

As for CanvasPixelData -- would using that preclude running the test in a JS shell?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Right; I suggested CanvasPixelArray for a reason; it's supported in all browsers, and is what would typically be used for image stuff...

And yes, that would preclude running the test in a JS shell.  :(

Also note that CanvasPixelArray has _very_ different behavior when doubles are assigned to it from either normal JS arrays or the typed Uint8 array.  So the benchmark as currently written would behave totally differently if it were used.

I agree that desaturating the same image over and over again doesn't seem like a useful test, btw.

Comment 4

[Robert Sayre]

(In reply to comment #0)
> Also, this characteristic stymies implementations that would like to specialize
> the array to an array-of-int... you could argue that this is a good test
> against overspecializing in such a case, but if the benchmark is supposed to be
> doing something useful, I'd argue against this.

I think the best way to handle that problem would be to find a well-written program that gets hurt by overspecializing.

(In reply to comment #2)
> 
> I think a lot of the Kraken benchmarks are very well suited to typed arrays,
> but Sayre didn't want to use them right now because they aren't widely
> supported.

I especially didn't want to use typed arrays in the first few iterations, because that might lead to the benchmark getting dismissed as unfair (to IE9 especially). I think it makes sense to switch to typed arrays for many kraken benchmarks once 3 of 5 browsers can use them.

Comment 5

[Nicholas Nethercote [inactive]]

There's a similar issue with imaging-gaussian-blur.  It only does the image transformation once, so it's better in that regard, but it would make sense if it truncated the fractional R/G/B/Alpha values before writing them back.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> And yes, that would preclude running the test in a JS shell.  :(

It seems, btw, that this is not the case.  The shell testcase could use a ClampedUint8Array, which is what CanvasPixelArray is in browsers.

Robert, I'm not sure using canvas imagedata (which is what I'm proposing for the browser version of the benchmark) for image manipulation would be dismissed as unfair to IE9.... It wouldn't work in IE8, though.

Comment 7

[Nicholas Nethercote [inactive]]

Pretty sure this isn't relevant any more, thanks to Kraken fading away and IonMonkey handling this sort of thing much better.