1258388 - jittests: compute job_count from the actual number of variants

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: numvar.patch

Some tests can have more modes to test than just the number of test_flags. For instance, some asm.js tests can be rune in --no-asmjs mode, which increases the number of variants under the hood, but doesn't increment job_count, resulting in weird cases like seen when runnning |jit_tests.py ./dist/bin/js asm.js|:

15:22 	<bbouvier>	am i the only observing jit-tests progress bar going over 100%? it just got to 132%
15:22 	<bbouvier>	i mean, i understand we're now focusing on quality, but passing tests at a 132% rate sounds like marketing

Note we can't just use len(job_list) cause job_list is a generator function; and we can't instantiate the whole list from job_list with len(list(job_list)) because some code thereafter assumes job_list is a generator...

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 8732877 [details] [diff] [review]
numvar.patch

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

Oh, so it looks like it was my patch in bug 1256699 broke this anyway.  Thanks for fixing.

::: js/src/tests/lib/jittests.py
@@ +161,5 @@
>          # For each list of jit flags, make a copy of the test.
>          return [self.copy_and_extend_jitflags(v) for v in variants]
>  
> +    def num_variants(self, variants):
> +        return len(self.copy_variants(variants))

I feel like this should just calculate the number rather than create the list and take the length.

I think it's |(len(variants) + len(self.test_also)) * (1 + len(self.test_join))| but that may be wrong.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jon Coppeard (:jonco) from comment #1)
> Comment on attachment 8732877 [details] [diff] [review]
> numvar.patch
> 
> Review of attachment 8732877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh, so it looks like it was my patch in bug 1256699 broke this anyway. 
> Thanks for fixing.
> 
> ::: js/src/tests/lib/jittests.py
> @@ +161,5 @@
> >          # For each list of jit flags, make a copy of the test.
> >          return [self.copy_and_extend_jitflags(v) for v in variants]
> >  
> > +    def num_variants(self, variants):
> > +        return len(self.copy_variants(variants))
> 
> I feel like this should just calculate the number rather than create the
> list and take the length.
> 
> I think it's |(len(variants) + len(self.test_also)) * (1 +
> len(self.test_join))| but that may be wrong.

Thank you for the review! I indeed think it is wrong: the loop with self.test_join multiplies the set by two at each iteration, so I *think* it should be (len(variants) + len(self.test_also)) * (2 ** len(self.test_join)).

That sounds like another argument for keeping it as is in the initial patch. If we take the formula approach,
- we'll need to sync up num_variants/copy_variants by hand
- => we might be incorrect when computing the value
- and probably calling copy_variants twice is cheap enough? Might need some more data here...

What do you think?

Comment 3

[Jon Coppeard (:jonco)]

Ah, you're right, we double it for every element in test_join.  Sorry about the misleading comment!

Probably the best thing to do is not use a generator at this stage, given that we have to compute everything anyway to get the correct length anyway.

How about setting test_list to a list containing all variants and then creating the generator based on that?

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/530267937f67

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/530267937f67