Bug 209964 - Re-enable previously skipped mips tests
Summary: Re-enable previously skipped mips tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Paulo Matos
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-03 07:19 PDT by Paulo Matos
Modified: 2020-04-21 02:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.26 KB, patch)
2020-04-03 07:21 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (6.17 KB, patch)
2020-04-14 04:42 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2020-04-15 03:56 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (6.12 KB, patch)
2020-04-16 01:11 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.26 KB, patch)
2020-04-16 01:59 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.27 KB, patch)
2020-04-16 23:27 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.26 KB, patch)
2020-04-17 05:58 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (5.27 KB, patch)
2020-04-17 06:02 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paulo Matos 2020-04-03 07:19:41 PDT
Re-enable previously skipped mips tests
Comment 1 Paulo Matos 2020-04-03 07:21:08 PDT
Created attachment 395373 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2020-04-03 11:39:11 PDT
Comment on attachment 395373 [details]
Patch

cq- since `jsc` tests are failing.  Can't land with tests failing.

<https://ews-build.webkit.org/#/builders/1/builds/10294>
Comment 3 Paulo Matos 2020-04-14 04:42:45 PDT
Created attachment 396400 [details]
Patch
Comment 4 Paulo Matos 2020-04-15 03:56:01 PDT
Created attachment 396518 [details]
Patch
Comment 5 David Kilzer (:ddkilzer) 2020-04-15 17:35:33 PDT
Comment on attachment 396518 [details]
Patch

This seems fine now, but I'd prefer to let a JSC expert review in case there's something subtle I'm missing.
Comment 6 Mark Lam 2020-04-15 23:38:17 PDT
Comment on attachment 396518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396518&action=review

> JSTests/stress/generator-cell-with-type.js:2
> +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)

Is there a reason the following wouldn't work?
//@ requireOptions("-e 'let hardness=1e5'") if ["mips"].include?($architecture)

I expect requireOptions(x, y, z) to read as the test needing options x, y, and z.  However, the way you have it above reads like requiring option "-e" and option "let hardness=1e5", which I think is weird.  Would be nice if we can avoid this.

> JSTests/stress/generator-cell-with-type.js:4
> +hardness = typeof(hardness) === 'undefined' ? 1e6 : hardness;

Let's just call this "iterations" instead "hardness".
Comment 7 Paulo Matos 2020-04-16 01:00:07 PDT
(In reply to Mark Lam from comment #6)
> Comment on attachment 396518 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396518&action=review
> 
> > JSTests/stress/generator-cell-with-type.js:2
> > +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)
> 
> Is there a reason the following wouldn't work?
> //@ requireOptions("-e 'let hardness=1e5'") if
> ["mips"].include?($architecture)
> 

That was the first thing I tried but it doesn't work:
Could not open file: -e 'let hardness=1e5'

Unfortunately when the script calls the command it passes the whole thing as a single argument and jsc seems to think it's a file. I looked into this earlier on when committed this r259114.

> I expect requireOptions(x, y, z) to read as the test needing options x, y,
> and z.  However, the way you have it above reads like requiring option "-e"
> and option "let hardness=1e5", which I think is weird.  Would be nice if we
> can avoid this.
> 
> > JSTests/stress/generator-cell-with-type.js:4
> > +hardness = typeof(hardness) === 'undefined' ? 1e6 : hardness;
> 
> Let's just call this "iterations" instead "hardness".


Sure - I will push a new patch straight away.
Comment 8 Paulo Matos 2020-04-16 01:11:33 PDT
Created attachment 396628 [details]
Patch
Comment 9 Paulo Matos 2020-04-16 01:59:38 PDT
Created attachment 396629 [details]
Patch
Comment 10 Mark Lam 2020-04-16 11:02:06 PDT
Comment on attachment 396629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396629&action=review

I noticed that you removed the change in jsc-run-stress-tests that added quoting around options with white spaces.  Did that turn out to not be needed after all?

> JSTests/stress/generator-cell-with-type.js:2
> +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)

Why keep the hardness name at all?  What does "hardness" mean?  What's happening here is that you're trying to override the iterations.  Why not just call it that?

> JSTests/stress/generator-cell-with-type.js:4
> +iterations = typeof(hardness) === 'undefined' ? 1e6 : hardness;

Ditto.
Comment 11 Paulo Matos 2020-04-16 23:24:47 PDT
(In reply to Mark Lam from comment #10)
> Comment on attachment 396629 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396629&action=review
> 
> I noticed that you removed the change in jsc-run-stress-tests that added
> quoting around options with white spaces.  Did that turn out to not be
> needed after all?
> 

Correct. That was for bytecode-cache tests, which unfortunately cannot run when we pass options with -e, so I removed it.

> > JSTests/stress/generator-cell-with-type.js:2
> > +//@ requireOptions("-e", "let hardness=1e5") if ["mips"].include?($architecture)
> 
> Why keep the hardness name at all?  What does "hardness" mean?  What's
> happening here is that you're trying to override the iterations.  Why not
> just call it that?
>

Ah - from your previous comment, I thought you wanted me to use hardness and iterations instead of just hardness. I understand now you just want to use iterations. In the mind of a non-native english speaker it means how hard something is supported to be... in any case, happy to use iterations.
 
> > JSTests/stress/generator-cell-with-type.js:4
> > +iterations = typeof(hardness) === 'undefined' ? 1e6 : hardness;
> 
> Ditto.

Sure.
Comment 12 Paulo Matos 2020-04-16 23:27:03 PDT
Created attachment 396742 [details]
Patch
Comment 13 Paulo Matos 2020-04-17 05:58:35 PDT
Created attachment 396759 [details]
Patch
Comment 14 Paulo Matos 2020-04-17 06:02:03 PDT
Created attachment 396760 [details]
Patch
Comment 15 Mark Lam 2020-04-17 07:13:16 PDT
Comment on attachment 396760 [details]
Patch

r=me
Comment 16 EWS 2020-04-21 02:52:50 PDT
Committed r260421: <https://trac.webkit.org/changeset/260421>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396760 [details].
Comment 17 Radar WebKit Bug Importer 2020-04-21 02:53:15 PDT
<rdar://problem/62098580>