filterPar produces incorrect result

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaswanth.sreeram, Assigned: shu)

Tracking

30 Branch
mozilla31
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
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]
Reporter

Updated

5 years ago
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Assignee

Comment 1

5 years ago
Posted 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)
Assignee

Comment 3

5 years ago
Posted patch Remove filterPar chunking. (obsolete) — Splinter Review
Attachment #8401668 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Duplicate of this bug: 844890
Assignee

Comment 5

5 years ago
Removed FIXME comment.
Attachment #8402021 - Attachment is obsolete: true
Attachment #8402022 - Flags: review?(nmatsakis)
Assignee

Updated

5 years ago
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: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1013056
Depends on: 1020800
No longer depends on: 1013056
Depends on: 1034383
You need to log in before you can comment on or make changes to this bug.