jittests: compute job_count from the actual number of variants

RESOLVED FIXED in Firefox 48

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8732877 [details] [diff] [review]
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...
Attachment #8732877 - Flags: review?(jcoppeard)
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.
Attachment #8732877 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 2

2 years ago
(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?
Flags: needinfo?(jcoppeard)
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?
Flags: needinfo?(jcoppeard)

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/530267937f67
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.