perf(tests): Spawn image_diff.py only once per job (#13013) c4ea178471 perf(goldens): Spawn image_diff.py only once per job diff.py was spawning a python process per image, and each re-imported cv2. This import absolutely DOMINATED the time spent diffing, especially on Windows. Instead, import cv2 once per job, and diff multiple images per image_diff.py process. Time improvement diffing gms on my laptop: 8.7 -> 2 seconds And diffing gms + goldens: 26 -> 10 seconds Co-authored-by: Chris Dalton <99840794+csmartdalton@users.noreply.github.com>
diff --git a/.rive_head b/.rive_head index 08f49f9..0d35efd 100644 --- a/.rive_head +++ b/.rive_head
@@ -1 +1 @@ -19d8fb7957c4a37b699b530b5def3f355e2c164e +c4ea1784715990dd1711c8cab3f79d6b59710581
diff --git a/tests/check_golds.sh b/tests/check_golds.sh index 9a660fb..6db0f7d 100755 --- a/tests/check_golds.sh +++ b/tests/check_golds.sh
@@ -150,12 +150,12 @@ python3 deploy_tests.py $TESTS $ARGS --target=$TARGET --outdir=.gold/$ID --backend=$BACKEND $NO_REBUILD else echo - echo "Checking $ID..." + echo "Deploying $ID..." rm -fr .gold/candidates/$ID python3 deploy_tests.py $TESTS $ARGS --target=$TARGET --outdir=.gold/candidates/$ID --backend=$BACKEND $NO_REBUILD echo - echo "Checking $ID..." + echo "Diffing $ID..." rm -fr .gold/diffs/$ID && mkdir -p .gold/diffs/$ID python3 diff.py $DIFF_ARGS -g .gold/$ID -c .gold/candidates/$ID -j$NUMBER_OF_PROCESSORS -o .gold/diffs/$ID \ || open_file .gold/diffs/$ID/index.html
diff --git a/tests/diff.py b/tests/diff.py index 80fd38b..30c83b9 100644 --- a/tests/diff.py +++ b/tests/diff.py
@@ -29,8 +29,8 @@ import argparse import glob import csv -from multiprocessing import Pool -from functools import partial +import threading +import queue from xml.etree import ElementTree as ET from typing import TypeVar import shutil @@ -54,9 +54,6 @@ args = parser.parse_args() -# _winapi.WaitForMultipleObjects only supports 64 handles, which we exceed if we span >61 diff jobs. -args.jobs = min(args.jobs, 61) - status_filename_base = "_imagediff_status" status_filename_pattern = f"{status_filename_base}_%i_*.txt" % os.getpid() show_commands = False @@ -237,11 +234,6 @@ for file in file_names: shutil.copyfile(file.path, os.path.join(dest, file.name)) -def remove_suffix(name, oldsuffix): - if name.endswith(oldsuffix): - name = name[:-len(oldsuffix)] - return name - def write_csv(entries, origpath, candidatepath, diffpath, missing_candidates): origpath = os.path.relpath(origpath, diffpath) candidatepath = os.path.relpath(candidatepath, diffpath) @@ -301,29 +293,44 @@ csv_writer.writerow({'type':'identical', 'number' : str(total_identical)}) csv_writer.writerow({'type':'total', 'number' : str(total_entries)}) -def call_imagediff(filename, golden, candidate, output, parent_pid): - cmd = [PYTHON, "image_diff.py", - "-n", remove_suffix(filename, ".png"), - "-g", os.path.join(golden, filename), - "-c", os.path.join(candidate, filename), +def diff_worker(index, work_queue, golden, candidate, output, parent_pid): + # One long-lived image_diff.py process that pulls filenames off the shared + # work_queue one at a time. Keeping the process alive means cv2 is imported + # only once (that import + process spawn dominate on Windows). + cmd = [PYTHON, "image_diff.py", "--serve", + "-g", golden, + "-c", candidate, # Each process writes its own status file in order to avoid race conditions. - "-s", "%s_%i_%i.txt" % (status_filename_base, parent_pid, os.getpid())] + "-s", "%s_%i_%i.txt" % (status_filename_base, parent_pid, index)] if output is not None: cmd.extend(["-o", output]) if args.verbose: cmd.extend(["-v", "-l"]) if args.histogram_compare: cmd.extend(["-H"]) - + if show_commands: - str = "" - for c in cmd: - str += c + " " - print(str) - - if 0 != subprocess.call(cmd): - print("Error calling " + cmd[0]) - return -1 + print(" ".join(cmd)) + + proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, + text=True) + try: + while True: + try: + filename = work_queue.get_nowait() + except queue.Empty: + break + proc.stdin.write(filename + "\n") + proc.stdin.flush() + # Block until the worker signals it finished this image. If the pipe + # closed early the worker died; stop feeding it (the missing status + # line is caught by the line-count check in diff_directory_shallow). + if proc.stdout.readline().strip() != "done": + print("image_diff.py worker %i died processing %s" % (index, filename)) + break + finally: + proc.stdin.close() + proc.wait() def parse_status(candidates_path, golden_path, output_path, device_name, browserstack_details): total_lines = 0 @@ -364,14 +371,26 @@ print("Diffing %i candidates..." % len(intersect_filenames)) sys.stdout.flush() -# generate the diffs (if any) and write to the status file - f = partial(call_imagediff, - golden=golden_path, - candidate=candidates_path, - output=output_path, - parent_pid=os.getpid()) - - Pool(args.jobs).map(f, intersect_filenames) + # Fill a shared queue with every filename. + work_queue = queue.Queue() + for filename in intersect_filenames: + work_queue.put(filename) + + # Generate the diffs (if any) and write to the status file(s). Start up to + # `jobs` long-lived image_diff.py workers that each pull filenames off the + # queue as fast as they can finish them. + njobs = min(args.jobs, len(intersect_filenames)) + parent_pid = os.getpid() + threads = [] + for index in range(njobs): + t = threading.Thread(target=diff_worker, + args=(index, work_queue, golden_path, + candidates_path, output_path, parent_pid)) + t.start() + threads.append(t) + for t in threads: + t.join() + (total_lines, entries, success) = parse_status(candidates_path, golden_path, output_path, device_name, browserstack_details) entries.extend(missing)
diff --git a/tests/image_diff.py b/tests/image_diff.py index bbd60f6..898f583 100644 --- a/tests/image_diff.py +++ b/tests/image_diff.py
@@ -4,49 +4,75 @@ import cv2 as cv import numpy as np import os +import sys parser = argparse.ArgumentParser(description= """ -Compares to images by pixels, outputing two different images for the differences. +Compares candidate images against goldens by pixels, writing one status line +per image and (optionally) two diff images per image. -output file {status} -output file {name}.diff0.png - The exact abs(A - B) of each pixel -output file {name}.diff1.png - A mask which has 1 for difference and 0 for no difference at a given pixel +A single process diffs multiple images in order to amortize the cost of spawning +the Python interpreter and importing OpenCV (cv2). + +Two ways to feed images to a process: + --names diff a fixed list of file names, then exit. + --serve stay alive reading one file name per line from stdin, writing + "done" to stdout after each, so the parent (diff.py) can pull work + dynamically and keep every worker busy until the queue is empty. + +output file {status} one status line per image +output file {name}.diff0.png the exact abs(A - B) of each pixel +output file {name}.diff1.png a mask which has 255 for difference, 0 otherwise """) -parser.add_argument("-n", "--name", type=str, required=True, help="name used for output images") -parser.add_argument("-c", "--candidate", type=str, required=True, help="candidate for image diffing") -parser.add_argument("-g", "--golden", type=str, required=True, help="golden to compare against") +mode = parser.add_mutually_exclusive_group(required=True) +mode.add_argument("-N", "--names", type=str, nargs='+', help="space-separated image file names (e.g. foo.png bar.png) to diff; each is looked up by name in the --candidate and --golden directories") +mode.add_argument("--serve", action='store_true', help="read image file names from stdin, one per line, diffing each and writing 'done' to stdout after each; exit on EOF") +parser.add_argument("-c", "--candidate", type=str, required=True, help="candidate DIRECTORY of images to diff") +parser.add_argument("-g", "--golden", type=str, required=True, help="golden DIRECTORY to compare against") parser.add_argument("-s", "--status", type=str, required=True, help="output stats file name and location") parser.add_argument("-o", "--outdir", type=str, help="output directory to store image diffs, if not provided then no output image is saved") parser.add_argument("-v", "--verbose", action='store_true', help="enable verbose logging") -parser.add_argument("-l", "--log", action='store_true', help="redirect all verbose logging to a file with --name") +parser.add_argument("-l", "--log", action='store_true', help="redirect all verbose logging to a per-image file") parser.add_argument("-H", "--histogram", action='store_true', help="compare images using histograms as an additional method for ruling out 'same' images. This should prevent subtle differences in the image that should not be visible from preventing a passing result") args = parser.parse_args() -def verbose_log(val): +def remove_suffix(name, oldsuffix): + if name.endswith(oldsuffix): + name = name[:-len(oldsuffix)] + return name + +def verbose_log(name, val): if not args.verbose: return if args.log: - with open(f"tmp/{args.name}.log", "a") as log_file: + with open(f"tmp/{name}.log", "a") as log_file: log_file.write(val+"\n") else: - print(val) + print(val, file=sys.stderr) -def main(): - if args.log: - os.makedirs("tmp", exist_ok=True) +def diff_one(filename, status): + name = remove_suffix(filename, ".png") + candidate_path = os.path.join(args.candidate, filename) + golden_path = os.path.join(args.golden, filename) + candidate = None + golden = None + diff = None + rgb_diff = None + mask = None + total_diff_count = 0 + max_diff = 0 + avg = 0.0 + hist_result = 1.0 size_match = False failed = False try: - verbose_log(f"loading {args.candidate}") - candidate = cv.imread(args.candidate) - verbose_log(f"loading {args.golden}") - golden = cv.imread(args.golden) + verbose_log(name, f"loading {candidate_path}") + candidate = cv.imread(candidate_path) + verbose_log(name, f"loading {golden_path}") + golden = cv.imread(golden_path) if candidate is not None and golden is not None: size_match = candidate.shape == golden.shape @@ -85,67 +111,99 @@ hist_candidate = cv.calcHist(hsv_candidate, [0,1], None, histSize, ranges, accumulate=False) hist_golden = cv.calcHist(hsv_golden, [0,1], None, histSize, ranges, accumulate=False) - # compare using CORREL histogram algorithm. the different options are detailed here + # compare using CORREL histogram algorithm. the different options are detailed here # https://docs.opencv.org/3.4/d6/dc7/group__imgproc__hist.html#ga994f53817d621e2e4228fc646342d386 # a hist_result of 1.0 means identical with this method. any variance results in a number less then 1.0 hist_result = cv.compareHist(hist_candidate, hist_golden, cv.HISTCMP_CORREL) - + except Exception as E: - print(f"Failed to load and process images {E}") + print(f"Failed to load and process images {E}", file=sys.stderr) failed = True + status.write(name + "\t") + if failed: + status.write("failed\n") + verbose_log(name, "failed to load golden or candidate") + return + if candidate is None: + status.write("missing_candidate\n") + verbose_log(name, "missing candidate for " + name) + return + if golden is None: + status.write("missing_golden\n") + verbose_log(name, "missing golden for " + name) + return + if failed: + status.write("failed\n") + verbose_log(name, "failed to load golden or candidate") + return + if total_diff_count == 0: + status.write("identical\n") + verbose_log(name, "files are identical") + return + if not size_match: + status.write("sizemismatch\n") + verbose_log(name, "files are not the same size") + return + status.write(str(max_diff)+"\t") + # prevent python from writing out in scientific notation + status.write(f"{float(avg):.5f}\t") + status.write(str(total_diff_count)+"\t") + status.write(str(diff.shape[0]*diff.shape[1])) + # add our histogram result as the last value of the status file or write a new line to say we are finished with this status + if args.histogram: + status.write(f"\t{float(hist_result):.5f}\n") + else: + status.write("\n") + + # save the output files if location provided + if args.outdir: + # color diff file location + c_diff_path = os.path.join(args.outdir, name + ".diff0.png") + # mask diff file location + c_mask_path = os.path.join(args.outdir, name + ".diff1.png") + verbose_log(name, f"writing images {c_diff_path} and {c_mask_path}") + cv.imwrite(c_diff_path, rgb_diff) + cv.imwrite(c_mask_path, mask) + verbose_log(name, "finished writing output images") + +def prepare_dirs(): + if args.log: + os.makedirs("tmp", exist_ok=True) + # make path to stats file if necessary if os.path.dirname(args.status): - verbose_log(f"making status file path {os.path.dirname(args.status)}") os.makedirs(os.path.dirname(args.status), exist_ok=True) - verbose_log(f"making status file {args.status}") - with open(args.status, "a") as status: - status.write(args.name + "\t"); - if candidate is None: - status.write("missing_candidate\n") - verbose_log("missing golden for " + args.name) - return - if golden is None: - status.write("missing_golden\n") - verbose_log("missing golden for " + args.name) - return - if failed: - status.write("failed\n") - verbose_log("failed to load golden or candidate") - return - if total_diff_count == 0: - status.write("identical\n") - verbose_log("files are identical") - return - if not size_match: - status.write("sizemismatch\n") - verbose_log("files are not the same size") - return - status.write(str(max_diff)+"\t") - # prevent python from writing out in scientific notation - status.write(f"{float(avg):.5f}\t") - status.write(str(total_diff_count)+"\t") - status.write(str(diff.shape[0]*diff.shape[1])) - # add our histogram result as the last value of the status file or write a new line to say we are finished with this status - if args.histogram: - status.write(f"\t{float(hist_result):.5f}\n") - else: - status.write("\n") - - verbose_log("status file finished") - # save the output file if location provided if args.outdir: - # color diff file location - c_diff_path = os.path.join(args.outdir, args.name + ".diff0.png") - # mask diff file location - c_mask_path = os.path.join(args.outdir, args.name + ".diff1.png") - # create output dir if needed - verbose_log(f"making output directory {args.outdir}") os.makedirs(args.outdir, exist_ok=True) - verbose_log(f"writing images {c_diff_path} and {c_mask_path}") - cv.imwrite(c_diff_path, rgb_diff) - cv.imwrite(c_mask_path, mask) - verbose_log("finished writing output images") + +def main(): + # A single process handles the whole list; open the status file once and + # append one line per image. + prepare_dirs() + with open(args.status, "a") as status: + for filename in args.names: + diff_one(filename, status) + +def serve(): + # Interactive worker: the parent (diff.py) keeps this process alive and + # feeds it one image file name per line on stdin, so cv2 is imported once + # and then reused across many images. After each image we append its status + # line to the status file and write "done" to stdout so the parent knows + # we're ready for the next name. Exit on EOF. + prepare_dirs() + with open(args.status, "a") as status: + for line in sys.stdin: + filename = line.strip() + if not filename: + continue + diff_one(filename, status) + status.flush() + sys.stdout.write("done\n") + sys.stdout.flush() if __name__ == '__main__': - main() + if args.serve: + serve() + else: + main()