Closed
Bug 1258388
Opened 8 years ago
Closed 8 years ago
jittests: compute job_count from the actual number of variants
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
Details
Attachments
(1 file)
1.81 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
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 1•8 years ago
|
||
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•8 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)
Comment 3•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/530267937f67
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•