Closed Bug 1456119 Opened 6 years ago Closed 6 years ago

Sort features in test262 update script

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(2 files)

Similar to bug 1408583, we should sort the feature tests to reduce differences between different Python versions.
Use list comprehension instead of sets to avoid non-deterministic sort order of the feature lists.
Attachment #8970282 - Flags: review?(sphink)
Rerun the python script to update the feature checks.
Attachment #8970286 - Flags: review?(sphink)
Comment on attachment 8970282 [details] [diff] [review]
bug1456119-part1-sorted.patch

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

::: js/src/tests/test262-update.py
@@ +242,5 @@
>          refTestSkip.append("not a test file")
>  
>      # Skip tests with unsupported features.
>      if "features" in testRec:
> +        unsupported = [f for f in testRec["features"] if f in UNSUPPORTED_FEATURES]

What?! You're changing from an O(n) algorithm to an O(n**2)? What if there are 20 features? Do you have *any* idea how long it takes a computer to count up to 400?

...oh, you do, do you?

Okay, this is fine, then.
Attachment #8970282 - Flags: review?(sphink) → review+
Attachment #8970286 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #3)
> >      if "features" in testRec:
> > +        unsupported = [f for f in testRec["features"] if f in UNSUPPORTED_FEATURES]
> 
> What?! You're changing from an O(n) algorithm to an O(n**2)? What if there
> are 20 features? Do you have *any* idea how long it takes a computer to
> count up to 400?
> 
> ...oh, you do, do you?
> 
> Okay, this is fine, then.

No worries, UNSUPPORTED_FEATURES is already a |set|, so we should still be in O(n). :-)
Doh! I'm not sure why I assumed it was a list. The fact that we were previously calling .intersection on it ought to have given me a rather large hint...
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12bfc2acf1d3
Part 1: Sort feature checks in test262 update script. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/b66c3bf69b40
Part 2: Reimport tests after features were sorted. r=sfink
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/12bfc2acf1d3
https://hg.mozilla.org/mozilla-central/rev/b66c3bf69b40
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.