745251 - Support the --jitflags= argument in jstests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

As mentioned in 745237:comment#1.

Comment 1

[Terrence Cole [:terrence]]

If you want to get started on this bug....

We parse options in the jstest suite here:
http://mxr.mozilla.org/mozilla-central/source/js/src/tests/jstests.py#55

Here is the flag for jitflags in the jittest suite:
http://mxr.mozilla.org/mozilla-central/source/js/src/jit-test/jit_test.py#427

Here is where the flag gets used:
http://mxr.mozilla.org/mozilla-central/source/js/src/jit-test/jit_test.py#495

You will need add a jitargs to the TestCase in jstests:
http://mxr.mozilla.org/mozilla-central/source/js/src/tests/tests.py#99

Don't implement copy on TestCase as it's done in the jittests, use:
http://docs.python.org/library/copy.html

And of course, whoever implements this will need to make use of the jitargs flag, but I'll leave that as an exercise to the reader.

Comment 2

[Dirkjan Ochtman (:djc)]

See also bug 638219, where I did a lot of work to get jstests and jittests closer (but it's annoyingly bitrotted now).

Comment 3

[Pavel Zaichenkov]

I decided to work on this bug and there is a question regarding implementation.

As I see, 'jittests' flag adds additional arguments for each test case. However, an attribute 'js_cmd_prefix' of TestCase class(where arguments are actually stored), is expanded over all test cases. So where additional arguments are expected to be stored?

Comment 4

[Terrence Cole [:terrence]]

(In reply to Pavel Zaichenkov from comment #3)
> I decided to work on this bug and there is a question regarding
> implementation.
> 
> As I see, 'jittests' flag adds additional arguments for each test case.
> However, an attribute 'js_cmd_prefix' of TestCase class(where arguments are
> actually stored), is expanded over all test cases. So where additional
> arguments are expected to be stored?

Yeah, we don't currently have this field in jstests.  It's also needed for 746836, which I'm working on now.  I opened bug 778383 and put a patch there which adds an |options| field for this purpose.

Comment 5

[Pavel Zaichenkov]

Attachment: Implementation of --jitflags= argument in jstests.

Please review my patch.

This depends on bug 778383, so it's important to apply a patch at first that adds an |options| field for TestCase instances.
 
I am not so sure about possible options in --jitflags. I just copied them from jit-test.py (these are -m, -a, -p, -d, -n). Correct me if I am wrong.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 647201 [details] [diff] [review]
Implementation of --jitflags= argument in jstests.

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

This looks very good, I just have a few small changes to request.

::: js/src/tests/jstests.py
@@ +83,5 @@
>      harness_og.add_option('-t', '--timeout', type=float, default=150.0,
>                            help='Set maximum time a test is allows to run (in seconds).')
>      harness_og.add_option('-a', '--args', dest='shell_args', default='',
>                            help='Extra args to pass to the JS shell.')
> +    harness_og.add_option('--jitflags', dest='jitflags', default='',

Good catch changing the default here!

Please remove the dest='jitflags' parameter: the options parser will infer the correct destination name automatically.

@@ +233,5 @@
>      test_dir = os.path.dirname(os.path.abspath(__file__))
>      test_list = manifest.load(test_dir, xul_tester)
>      skip_list = []
>  
> +    # Create a new test list. Each JIT configuration is applied to the each test.

Let's write this in the active tense.  How about something like: "Apply each JIT configuration to every test."

@@ +235,5 @@
>      skip_list = []
>  
> +    # Create a new test list. Each JIT configuration is applied to the each test.
> +    if options.jitflags:
> +        from copy import copy

Move this import to the top level, just under "import os, sys, textwrap".

@@ +239,5 @@
> +        from copy import copy
> +        new_test_list = []
> +        jitflags_list = parse_jitflags(options.jitflags)
> +        if (options.shell_args):
> +            jitflags_list = remove_arg_dups(options.shell_args, jitflags_list)

There is no harm in having duplicate args (I'm not sure why jit-test bothers removing dups) and the existing code is not terribly good at removing duplicate test runs either.  For example, you can trick it easily with --jitflags=mn,nm.  I think for now we should give the most trivial (and therefore obvious and easy-to-understand) behavior and modify/optimize it later if it becomes a problem. So, for now, please just remove remove_arg_dups.

@@ +241,5 @@
> +        jitflags_list = parse_jitflags(options.jitflags)
> +        if (options.shell_args):
> +            jitflags_list = remove_arg_dups(options.shell_args, jitflags_list)
> +        for test in test_list:
> +          for jitflag in jitflags_list:

Please use the plural "jitflags" like jit-tests does.

@@ +243,5 @@
> +            jitflags_list = remove_arg_dups(options.shell_args, jitflags_list)
> +        for test in test_list:
> +          for jitflag in jitflags_list:
> +            tmp_test = copy(test)
> +            tmp_test.options = jitflag

Please use list.extend here, like jit-tests does.  The test headers add things to the options list that need to not get cleared here.

@@ +245,5 @@
> +          for jitflag in jitflags_list:
> +            tmp_test = copy(test)
> +            tmp_test.options = jitflag
> +            new_test_list.append(tmp_test)
> +        test_list = new_test_list

The whole of this new block needs to come after the manifest.make_manifests block below it: the manifest is for running the tests in the browser, so shell args aren't important there.

Comment 7

[Pavel Zaichenkov]

Attachment: Patch modification

I improved patch taking all your remarks into the account.

However, let me clarify why I'm advocating the necessity of duplicate arguments avoidance. 
Assume are running tests like "--jitflags=am,amd --args='-a'". In this case every test will be copied twice and there is no harm in having extra '-a' in argument list.
But if we have "--jitflags=am,amd --args='-a,-m'", we can copy every test only once, as 'am' jit flags can be eliminated.
So my point is that we should think about removing duplicate arguments to minimize number of test cases. Possibly my code wasn't perfect enough, so I can think more thoroughly about this.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 648977 [details] [diff] [review]
Patch modification

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

(In reply to Pavel Zaichenkov from comment #7)
> I improved patch taking all your remarks into the account.

It looks excellent, just one minor nit to take care of before landing.
 
> So my point is that we should think about removing duplicate arguments to
> minimize number of test cases. Possibly my code wasn't perfect enough, so I
> can think more thoroughly about this.

I think the code you had would have mostly worked, I just want to minimize deviations from jit-tests at this point to reduce confusion as people switch over to the new suite.  Could you  open a new bug blocking bug 745230 to implement this?

::: js/src/tests/jstests.py
@@ +232,5 @@
> +    if options.jitflags:
> +        new_test_list = []
> +        jitflags_list = parse_jitflags(options.jitflags)
> +        for test in test_list:
> +          for jitflags in jitflags_list:

This needs to be indented 4 spaces.

@@ +236,5 @@
> +          for jitflags in jitflags_list:
> +            tmp_test = copy(test)
> +            tmp_test.options = copy(test.options)
> +            tmp_test.options.extend(jitflags)
> +            new_test_list.append(tmp_test)

This block also needs 4 space indenting.

Comment 9

[Pavel Zaichenkov]

Attachment: Indentation fixed

Fixed indentation

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 649219 [details] [diff] [review]
Indentation fixed

Once a patch has been r+'ed, it is fine to carry the r+ forward to patches with the nits fixed: it's why we call them 'nits' and not r- :-).

The next step is to add checkin-needed to the keywords in the bug.  The build peers look for checkin-needed bugs and will check in the r+ patch for you until you get your own commit access.  At the same time you should also get commit access for yourself.  The directions are here: http://www.mozilla.org/hacking/committer/.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=80d24fa10c77

https://hg.mozilla.org/integration/mozilla-inbound/rev/9808b7c10bc7

Thanks for the patch, Pavel!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9808b7c10bc7

Comment 13

[Pavel Zaichenkov]

Terrence or Ryan,

Could you please provide a voucher for me in bug 782026?