Closed Bug 974201 Opened 7 years ago Closed 7 years ago

filterPar produces incorrect result

Categories

(Core :: JavaScript Engine: JIT, defect)

30 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jaswanth.sreeram, Assigned: shu)

References

Details

Attachments

(1 file, 2 obsolete files)

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]
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Attached patch Fix filterPar chunking. (obsolete) — Splinter Review
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.
Attachment #8401668 - Flags: review?(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. :)
Attachment #8401668 - Flags: review?(nmatsakis)
Attached patch Remove filterPar chunking. (obsolete) — Splinter Review
Attachment #8401668 - Attachment is obsolete: true
Duplicate of this bug: 844890
Removed FIXME comment.
Attachment #8402021 - Attachment is obsolete: true
Attachment #8402022 - Flags: review?(nmatsakis)
Assignee: nobody → shu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8402022 - Flags: review?(nmatsakis) → review+
https://hg.mozilla.org/mozilla-central/rev/7a1c696cade6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.