From 48e502e1511e63b57ef5e4dc5263b0a253be528b Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Mon, 29 Apr 2024 15:49:57 +0200 Subject: [PATCH 1/4] feat: Optimize gc cycles and introduce flag for reduced memoryusage --- bin/Color.ml | 46 ++++++++++++++++++++++++++------------------- bin/Main.ml | 28 +++++++++++++++++++++++---- bin/ODiffBin.ml | 9 ++++++++- src/Diff.ml | 34 ++++++++++++++------------------- test/Test_Core.ml | 11 +++++++---- test/Test_IO_PNG.ml | 8 ++------ 6 files changed, 82 insertions(+), 54 deletions(-) diff --git a/bin/Color.ml b/bin/Color.ml index 3c7d39da..c16cc388 100644 --- a/bin/Color.ml +++ b/bin/Color.ml @@ -1,23 +1,31 @@ let ofHexString s = match String.length s with - | (4 | 7) as len -> ( - let short = len = 4 in - let r' = - match short with true -> String.sub s 1 1 | false -> String.sub s 1 2 - in - let g' = - match short with true -> String.sub s 2 1 | false -> String.sub s 3 2 - in - let b' = - match short with true -> String.sub s 3 1 | false -> String.sub s 5 2 - in - let r = int_of_string_opt ("0x" ^ r') in - let g = int_of_string_opt ("0x" ^ g') in - let b = int_of_string_opt ("0x" ^ b') in + | (4 | 7) as len -> + (let short = len = 4 in + let r' = + match short with true -> String.sub s 1 1 | false -> String.sub s 1 2 + in + let g' = + match short with true -> String.sub s 2 1 | false -> String.sub s 3 2 + in + let b' = + match short with true -> String.sub s 3 1 | false -> String.sub s 5 2 + in + let r = int_of_string_opt ("0x" ^ r') in + let g = int_of_string_opt ("0x" ^ g') in + let b = int_of_string_opt ("0x" ^ b') in - match (r, g, b) with - | Some r, Some g, Some b when short -> - Some ((16 * r) + r, (16 * g) + g, (16 * b) + b) - | Some r, Some g, Some b -> Some (r, g, b) - | _ -> None) + match (r, g, b) with + | Some r, Some g, Some b when short -> + Some ((16 * r) + r, (16 * g) + g, (16 * b) + b) + | Some r, Some g, Some b -> Some (r, g, b) + | _ -> None) + |> Option.map (fun (r, g, b) -> + (* Create rgba pixel value right after parsing *) + let r = (r land 255) lsl 0 in + let g = (g land 255) lsl 8 in + let b = (b land 255) lsl 16 in + let a = 255 lsl 24 in + + Int32.of_int (a lor b lor g lor r)) | _ -> None diff --git a/bin/Main.ml b/bin/Main.ml index d99e1da9..9fc6b77c 100644 --- a/bin/Main.ml +++ b/bin/Main.ml @@ -14,7 +14,23 @@ type 'output diffResult = { exitCode : int; diff : 'output option } (* Arguments must remain positional for the cmd parser lib that we use *) let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange diffColorHex toEmitStdoutParsableString antialiasing ignoreRegions diffLines - = + disableMemoryOptimizations = + (* + We do not need to actually maintain memory size of the allocated RAM by odiff, so we are + increasing the minor memory size to avoid most of the possible deallocations. For sure it is + not possible be sure that it won't be run in OCaml because we allocate the Stack and Queue + + By default set the minor heap size to 256mb on 64bit machine + *) + if not disableMemoryOptimizations then + Gc.set + { + (Gc.get ()) with + Gc.minor_heap_size = 64_000_000; + Gc.stack_limit = 2_048_000; + Gc.window_size = 25; + }; + let module IO1 = (val getIOModule img1Path) in let module IO2 = (val getIOModule img2Path) in let module Diff = MakeDiff (IO1) (IO2) in @@ -24,9 +40,9 @@ let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange Diff.diff img1 img2 ~outputDiffMask ~threshold ~failOnLayoutChange ~antialiasing ~ignoreRegions ~diffLines ~diffPixel: - (Color.ofHexString diffColorHex |> function - | Some col -> col - | None -> (255, 0, 0)) + (match Color.ofHexString diffColorHex with + | Some c -> c + | None -> redPixel) () |> Print.printDiffResult toEmitStdoutParsableString |> function @@ -43,4 +59,8 @@ let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange (match diff with | Some output when outputDiffMask -> IO1.freeImage output | _ -> ()); + + Gc.print_stat stdout; + flush stdout; + Unix.sleep 40; exit exitCode diff --git a/bin/ODiffBin.ml b/bin/ODiffBin.ml index 11aaf8ab..14214045 100644 --- a/bin/ODiffBin.ml +++ b/bin/ODiffBin.ml @@ -56,6 +56,13 @@ let diffLines = "With this flag enabled, output result in case of different images \ will output lines for all the different pixels" +let reduceMemory = + value & flag + & info [ "reduce-ram-usage" ] + ~doc: + "With this flag enabled odiff will use less memory, but will be slower \ + in some cases." + let ignoreRegions = value & opt @@ -76,7 +83,7 @@ let cmd = in ( const Main.main $ base $ comp $ diffPath $ threshold $ diffMask $ failOnLayout $ diffColor $ parsableOutput $ antialiasing $ ignoreRegions - $ diffLines, + $ diffLines $ reduceMemory, Term.info "odiff" ~version:"3.0.0" ~doc:"Find difference between 2 images." ~exits: (Term.exit_info 0 ~doc:"on image match" diff --git a/src/Diff.ml b/src/Diff.ml index 9564b22f..ddb186dc 100644 --- a/src/Diff.ml +++ b/src/Diff.ml @@ -1,4 +1,7 @@ -let redPixel = (255, 0, 0) +(* Decimal representation of the RGBA in32 pixel red pixel *) +let redPixel = Int32.of_int 4278190335 + +(* Decimal representation of the RGBA in32 pixel green pixel *) let maxYIQPossibleDelta = 35215. type 'a diffVariant = Layout | Pixel of ('a * int * float * int Stack.t) @@ -18,18 +21,21 @@ module MakeDiff (IO1 : ImageIO.ImageIO) (IO2 : ImageIO.ImageIO) = struct let compare (base : IO1.t ImageIO.img) (comp : IO2.t ImageIO.img) ?(antialiasing = false) ?(outputDiffMask = false) ?(diffLines = false) - ?(diffPixel : int * int * int = redPixel) ?(threshold = 0.1) - ?(ignoreRegions = []) () = + ?diffPixel ?(threshold = 0.1) ?(ignoreRegions = []) () = let maxDelta = maxYIQPossibleDelta *. (threshold ** 2.) in + let diffPixel = match diffPixel with Some x -> x | None -> redPixel in let diffOutput = match outputDiffMask with | true -> IO1.makeSameAsLayout base | false -> base in - let diffPixelQueue = Queue.create () in + + let diffCount = ref 0 in let diffLinesStack = Stack.create () in let countDifference x y = - diffPixelQueue |> Queue.push (x, y); + incr diffCount; + IO1.setImgColor ~x ~y diffPixel diffOutput; + if diffLines && (diffLinesStack |> Stack.is_empty || diffLinesStack |> Stack.top < y) @@ -74,26 +80,14 @@ module MakeDiff (IO1 : ImageIO.ImageIO) (IO2 : ImageIO.ImageIO) = struct else incr x done; - let diffCount = diffPixelQueue |> Queue.length in - (if diffCount > 0 then - let r, g, b = diffPixel in - let a = (255 land 255) lsl 24 in - let b = (b land 255) lsl 16 in - let g = (g land 255) lsl 8 in - let r = (r land 255) lsl 0 in - let diffPixel = Int32.of_int (a lor b lor g lor r) in - diffPixelQueue - |> Queue.iter (fun (x, y) -> - diffOutput |> IO1.setImgColor ~x ~y diffPixel)); - let diffPercentage = - 100.0 *. Float.of_int diffCount + 100.0 *. Float.of_int !diffCount /. (Float.of_int base.width *. Float.of_int base.height) in - (diffOutput, diffCount, diffPercentage, diffLinesStack) + (diffOutput, !diffCount, diffPercentage, diffLinesStack) let diff (base : IO1.t ImageIO.img) (comp : IO2.t ImageIO.img) ~outputDiffMask - ?(threshold = 0.1) ?(diffPixel = redPixel) ?(failOnLayoutChange = true) + ?(threshold = 0.1) ~diffPixel ?(failOnLayoutChange = true) ?(antialiasing = false) ?(diffLines = false) ?(ignoreRegions = []) () = if failOnLayoutChange = true diff --git a/test/Test_Core.ml b/test/Test_Core.ml index bfcd24a4..d8c4dba1 100644 --- a/test/Test_Core.ml +++ b/test/Test_Core.ml @@ -13,8 +13,8 @@ let _ = PNG_Diff.compare img1 img2 ~outputDiffMask:false ~antialiasing:true () in - (expect.int diffPixels).toBe 38; - (expect.float diffPercentage).toBeCloseTo 0.095); + (expect.int diffPixels).toBe 46; + (expect.float diffPercentage).toBeCloseTo 0.115); test "tests different sized AA images" (fun { expect; _ } -> let img1 = loadImage "test/test-images/aa/antialiasing-on.png" in let img2 = @@ -58,14 +58,17 @@ let _ = let _ = describe "CORE: Diff Color" (fun { test; _ } -> - test "creates diff output image with custom diff color" + test "creates diff output image with custom green diff color" (fun { expect; _ } -> let img1 = Png.IO.loadImage "test/test-images/png/orange.png" in let img2 = Png.IO.loadImage "test/test-images/png/orange_changed.png" in let diffOutput, _, _, _ = - PNG_Diff.compare img1 img2 ~diffPixel:(0, 255, 0) () + PNG_Diff.compare img1 img2 + ~diffPixel: + (Int32.of_int 4278255360 (*int32 representation of #00ff00*)) + () in let originalDiff = Png.IO.loadImage "test/test-images/png/orange_diff_green.png" diff --git a/test/Test_IO_PNG.ml b/test/Test_IO_PNG.ml index 075ade37..aaf370ff 100644 --- a/test/Test_IO_PNG.ml +++ b/test/Test_IO_PNG.ml @@ -42,11 +42,7 @@ let _ = test "Correctly handles different encodings of transparency" (fun { expect; _ } -> - let img1 = - loadImage "test/test-images/png/extreme-alpha.png" - in - let img2 = - loadImage "test/test-images/png/extreme-alpha-1.png" - in + let img1 = loadImage "test/test-images/png/extreme-alpha.png" in + let img2 = loadImage "test/test-images/png/extreme-alpha-1.png" in let _, diffPixels, _, _ = Diff.compare img1 img2 () in (expect.int diffPixels).toBe 0)) From e0e216b3280cc96a09a432619e1a5efc8dd0a2f4 Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Mon, 29 Apr 2024 15:54:30 +0200 Subject: [PATCH 2/4] Update readme --- README.md | 2 ++ bin/Main.ml | 6 +++--- bin/ODiffBin.ml | 4 ++-- bin/node-bindings/odiff.d.ts | 2 ++ bin/node-bindings/odiff.js | 4 ++++ 5 files changed, 13 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ae48c616..5cbe090b 100644 --- a/README.md +++ b/README.md @@ -111,6 +111,8 @@ export type ODiffOptions = Partial<{ antialiasing: boolean; /** If `true` reason: "pixel-diff" output will contain the set of line indexes containing different pixels */ captureDiffLines: boolean; + /** If `true` odiff will use less memory but will be slower with larger images */ + reduceRamUsage: boolean; /** An array of regions to ignore in the diff. */ ignoreRegions: Array<{ x1: number; diff --git a/bin/Main.ml b/bin/Main.ml index 9fc6b77c..31109495 100644 --- a/bin/Main.ml +++ b/bin/Main.ml @@ -14,7 +14,7 @@ type 'output diffResult = { exitCode : int; diff : 'output option } (* Arguments must remain positional for the cmd parser lib that we use *) let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange diffColorHex toEmitStdoutParsableString antialiasing ignoreRegions diffLines - disableMemoryOptimizations = + disableGcOptimizations = (* We do not need to actually maintain memory size of the allocated RAM by odiff, so we are increasing the minor memory size to avoid most of the possible deallocations. For sure it is @@ -22,11 +22,11 @@ let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange By default set the minor heap size to 256mb on 64bit machine *) - if not disableMemoryOptimizations then + if not disableGcOptimizations then Gc.set { (Gc.get ()) with - Gc.minor_heap_size = 64_000_000; + Gc.minor_heap_size = 32_000_000; Gc.stack_limit = 2_048_000; Gc.window_size = 25; }; diff --git a/bin/ODiffBin.ml b/bin/ODiffBin.ml index 14214045..200d160c 100644 --- a/bin/ODiffBin.ml +++ b/bin/ODiffBin.ml @@ -56,7 +56,7 @@ let diffLines = "With this flag enabled, output result in case of different images \ will output lines for all the different pixels" -let reduceMemory = +let disableGcOptimizations = value & flag & info [ "reduce-ram-usage" ] ~doc: @@ -83,7 +83,7 @@ let cmd = in ( const Main.main $ base $ comp $ diffPath $ threshold $ diffMask $ failOnLayout $ diffColor $ parsableOutput $ antialiasing $ ignoreRegions - $ diffLines $ reduceMemory, + $ diffLines $ disableGcOptimizations, Term.info "odiff" ~version:"3.0.0" ~doc:"Find difference between 2 images." ~exits: (Term.exit_info 0 ~doc:"on image match" diff --git a/bin/node-bindings/odiff.d.ts b/bin/node-bindings/odiff.d.ts index 886ba4b2..da12b132 100644 --- a/bin/node-bindings/odiff.d.ts +++ b/bin/node-bindings/odiff.d.ts @@ -13,6 +13,8 @@ export type ODiffOptions = Partial<{ antialiasing: boolean; /** If `true` reason: "pixel-diff" output will contain the set of line indexes containing different pixels */ captureDiffLines: boolean; + /** If `true` odiff will use less memory but will be slower with larger images */ + reduceRamUsage: boolean; /** An array of regions to ignore in the diff. */ ignoreRegions: Array<{ x1: number; diff --git a/bin/node-bindings/odiff.js b/bin/node-bindings/odiff.js index 20c4d311..b78815f7 100644 --- a/bin/node-bindings/odiff.js +++ b/bin/node-bindings/odiff.js @@ -50,6 +50,10 @@ function optionsToArgs(options) { setFlag("output-diff-lines", value); break; + case "reduceRamUsage": + setFlag("reduceRamUsage", value); + break; + case "ignoreRegions": { const regions = value .map( From 490faadc67f8b58a64a9f47f1205ec87e0549387 Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Mon, 29 Apr 2024 16:01:24 +0200 Subject: [PATCH 3/4] Disable ram optimization in debug test --- bin/Main.ml | 6 ++---- bin/node-bindings/odiff.js | 2 +- test/node-binding.test.cjs | 44 ++++++++++++++++++++++---------------- 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/bin/Main.ml b/bin/Main.ml index 31109495..59bf290f 100644 --- a/bin/Main.ml +++ b/bin/Main.ml @@ -26,7 +26,7 @@ let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange Gc.set { (Gc.get ()) with - Gc.minor_heap_size = 32_000_000; + Gc.minor_heap_size = 64_000_000; Gc.stack_limit = 2_048_000; Gc.window_size = 25; }; @@ -60,7 +60,5 @@ let main img1Path img2Path diffPath threshold outputDiffMask failOnLayoutChange | Some output when outputDiffMask -> IO1.freeImage output | _ -> ()); - Gc.print_stat stdout; - flush stdout; - Unix.sleep 40; + (*Gc.print_stat stdout;*) exit exitCode diff --git a/bin/node-bindings/odiff.js b/bin/node-bindings/odiff.js index b78815f7..4efe3706 100644 --- a/bin/node-bindings/odiff.js +++ b/bin/node-bindings/odiff.js @@ -51,7 +51,7 @@ function optionsToArgs(options) { break; case "reduceRamUsage": - setFlag("reduceRamUsage", value); + setFlag("reduce-ram-usage", value); break; case "ignoreRegions": { diff --git a/test/node-binding.test.cjs b/test/node-binding.test.cjs index d3ef5425..9ae2a839 100644 --- a/test/node-binding.test.cjs +++ b/test/node-binding.test.cjs @@ -16,13 +16,31 @@ const BINARY_PATH = path.resolve( console.log(`Testing binary ${BINARY_PATH}`); +const options = { + __binaryPath: BINARY_PATH, +} + test("Outputs correct parsed result when images different", async (t) => { + const { reason, diffCount, diffPercentage } = await compare( + path.join(IMAGES_PATH, "donkey.png"), + path.join(IMAGES_PATH, "donkey-2.png"), + path.join(IMAGES_PATH, "diff.png"), + options + ); + + t.is(reason, "pixel-diff"); + t.is(diffCount, 109861); + t.is(diffPercentage, 2.85952484323); +}) + +test("Correctly works with reduceRamUsage", async (t) => { const { reason, diffCount, diffPercentage } = await compare( path.join(IMAGES_PATH, "donkey.png"), path.join(IMAGES_PATH, "donkey-2.png"), path.join(IMAGES_PATH, "diff.png"), { - __binaryPath: BINARY_PATH, + ...options, + reduceRamUsage: true, } ); @@ -36,10 +54,7 @@ test("Correctly parses threshold", async (t) => { path.join(IMAGES_PATH, "donkey.png"), path.join(IMAGES_PATH, "donkey-2.png"), path.join(IMAGES_PATH, "diff.png"), - { - threshold: 0.6, - __binaryPath: BINARY_PATH, - } + options ); t.is(reason, "pixel-diff"); @@ -52,10 +67,7 @@ test("Correctly parses antialiasing", async (t) => { path.join(IMAGES_PATH, "donkey.png"), path.join(IMAGES_PATH, "donkey-2.png"), path.join(IMAGES_PATH, "diff.png"), - { - antialiasing: true, - __binaryPath: BINARY_PATH, - } + options ); t.is(reason, "pixel-diff"); @@ -69,6 +81,7 @@ test("Correctly parses ignore regions", async (t) => { path.join(IMAGES_PATH, "donkey-2.png"), path.join(IMAGES_PATH, "diff.png"), { + ...options, ignoreRegions: [ { x1: 749, @@ -83,7 +96,6 @@ test("Correctly parses ignore regions", async (t) => { y2: 1334, }, ], - __binaryPath: BINARY_PATH, } ); @@ -95,9 +107,7 @@ test("Outputs correct parsed result when images different for cypress image", as path.join(IMAGES_PATH, "www.cypress.io.png"), path.join(IMAGES_PATH, "www.cypress.io-1.png"), path.join(IMAGES_PATH, "diff.png"), - { - __binaryPath: BINARY_PATH, - } + options ); t.is(reason, "pixel-diff"); @@ -110,9 +120,7 @@ test("Correctly handles same images", async (t) => { path.join(IMAGES_PATH, "donkey.png"), path.join(IMAGES_PATH, "donkey.png"), path.join(IMAGES_PATH, "diff.png"), - { - __binaryPath: BINARY_PATH, - } + options ); t.is(match, true); @@ -125,7 +133,7 @@ test("Correctly outputs diff lines", async (t) => { path.join(IMAGES_PATH, "diff.png"), { captureDiffLines: true, - __binaryPath: BINARY_PATH, + ...options } ); @@ -139,8 +147,8 @@ test("Returns meaningful error if file does not exist and noFailOnFsErrors", asy path.join(IMAGES_PATH, "not-existing.png"), path.join(IMAGES_PATH, "diff.png"), { + ...options, noFailOnFsErrors: true, - __binaryPath: BINARY_PATH, } ); From b81400a0ddfce5b95b0d9f9b0dff78168828e175 Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Mon, 29 Apr 2024 16:24:43 +0200 Subject: [PATCH 4/4] Fix node bindgins tests --- test/node-binding.test.cjs | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/test/node-binding.test.cjs b/test/node-binding.test.cjs index 9ae2a839..81733445 100644 --- a/test/node-binding.test.cjs +++ b/test/node-binding.test.cjs @@ -29,8 +29,8 @@ test("Outputs correct parsed result when images different", async (t) => { ); t.is(reason, "pixel-diff"); - t.is(diffCount, 109861); - t.is(diffPercentage, 2.85952484323); + t.is(diffCount, 101841); + t.is(diffPercentage, 2.65077570347); }) test("Correctly works with reduceRamUsage", async (t) => { @@ -45,8 +45,8 @@ test("Correctly works with reduceRamUsage", async (t) => { ); t.is(reason, "pixel-diff"); - t.is(diffCount, 109861); - t.is(diffPercentage, 2.85952484323); + t.is(diffCount, 101841); + t.is(diffPercentage, 2.65077570347); }); test("Correctly parses threshold", async (t) => { @@ -54,12 +54,15 @@ test("Correctly parses threshold", async (t) => { path.join(IMAGES_PATH, "donkey.png"), path.join(IMAGES_PATH, "donkey-2.png"), path.join(IMAGES_PATH, "diff.png"), - options + { + ...options, + threshold: 0.5, + } ); t.is(reason, "pixel-diff"); - t.is(diffCount, 50332); - t.is(diffPercentage, 1.31007003768); + t.is(diffCount, 65357); + t.is(diffPercentage, 1.70114931758); }); test("Correctly parses antialiasing", async (t) => { @@ -67,12 +70,15 @@ test("Correctly parses antialiasing", async (t) => { path.join(IMAGES_PATH, "donkey.png"), path.join(IMAGES_PATH, "donkey-2.png"), path.join(IMAGES_PATH, "diff.png"), - options + { + ...options, + antialiasing: true, + } ); t.is(reason, "pixel-diff"); - t.is(diffCount, 108208); - t.is(diffPercentage, 2.8164996153); + t.is(diffCount, 101499); + t.is(diffPercentage, 2.64187393218); }); test("Correctly parses ignore regions", async (t) => { @@ -138,7 +144,7 @@ test("Correctly outputs diff lines", async (t) => { ); t.is(match, false); - t.is(diffLines.length, 411); + t.is(diffLines.length, 402); }); test("Returns meaningful error if file does not exist and noFailOnFsErrors", async (t) => {