974201 - filterPar produces incorrect result

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jaswanth Sreeram]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140218030202

Steps to reproduce:

Open JavaScript console in Nightly 30.0a1 (2014-02-18).
Type the following and run:

var r = [1,2,3,4].filterPar((e, i)=>{return i%2 ? false : true;}); r;



Actual results:

The following is printed:

[1]


Expected results:

The following should be printed:

[1, 3]

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Fix filterPar chunking.

The bug is that I ported over the old filterPar chunking code without thinking
about it. Now that slice sizes are fluid, we won't get nicely 32-element
aligned chunks. This updates the scheme.

Comment 2

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8401668 [details] [diff] [review]
Fix filterPar chunking.

Review of attachment 8401668 [details] [diff] [review]:
-----------------------------------------------------------------

Two suggestions for making the code harder to follow. That said, it might be worth just scrapping the whole bitset thing and using an array of bools -- the bitset was natural before but now it feels a bit like premature optimization. I'll leave it to your judgement though.

::: js/src/builtin/Array.js
@@ +985,5 @@
>        var indexStart = SLICE_START_INDEX(sliceShift, sliceId);
>        var indexEnd = SLICE_END_INDEX(sliceShift, indexStart, length);
> +      var chunkPos = sliceId * chunkMultiplier;
> +      var chunkBits = 0;
> +      for (var bit = 0, indexPos = indexStart; indexPos < indexEnd; bit++, indexPos++) {

Can we iterate over the chunks first and then the indices within each chunk? I find the loop structure here a bit hard to follow, though not nearly so bad as the other case.

@@ +1037,5 @@
>  
> +          // If no bits are set, we can skip the entire chunk.
> +          if (chunkBits === 0) {
> +            indexPos += 32;
> +            continue;

This loop is pretty hard to read the way it's written. Overall, I think it'd be better to restructure this loop into two loops, like it was before. Iterate over the chunks and then iterate within each chunk over the indices. (Case in point, I think there might be an off by one error here. You increment indexPos by 32, but then the loop will increment it again, no? OTOH, I'd expect filterVeryFew to expose this, so I could be wrong.)

Also, skipping an empty chunk when bits == 32 misses the case where the first chunk is entirely empty. As long as we're going to microoptimize, might as well get every last bit. :)

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Remove filterPar chunking.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Remove filterPar chunking.

Removed FIXME comment.

Comment 6

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1c696cade6

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7a1c696cade6