Optimise PropertyFilter.fromString (#63)

* Optimise PropertyFilter.fromString

* Codecov: ignore __benchmark__

* Add tests

* Codecov: ignore __benchmark__ try #2

* Fix codecov & validate structure

---------

Co-authored-by: Oskar Dahlin <oskar.dahlin@divid.se>

codecov.yml

@@ -4,19 +4,19 @@ coverage:
     project:
       default: off
       property:
-        flags: property
+        flags: [property]
         target: 64%
       property_filter_pretty:
         target: 66%
-        flags: property_filter_pretty
+        flags: [property_filter_pretty]
       react_properties_selector:
         target: 66%
-        flags: react_properties_selector
+        flags: [react_properties_selector]
       react_property_selectors:
-        flags: react_property_selectors
+        flags: [react_property_selectors]
         target: 26%
       variant_listing:
-        flags: variant_listing
+        flags: [variant_listing]
         target: 78%
 flags:
   property:
@@ -31,3 +31,8 @@ flags:
   variant_listing:
     paths:
       - packages/variant-listing/src
+ignore:
+  - "**/__benchmarks__/**"
+
+# When modifying this file, please validate using
+# curl -X POST --data-binary @codecov.yml https://codecov.io/validate

package.json

@@ -72,6 +72,7 @@
     "build-storybook": "build-storybook",
     "benchmark": "yarn build && run-s benchmark:*",
     "benchmark:property": "node packages/property/lib/__benchmarks__/property-filter.js",
+    "benchmark:parse": "node packages/property/lib/__benchmarks__/property-filter-parse.js",
     "test": "run-s build test-all",
     "test-all": "jest",
     "test:property": "jest --projects packages/property",

packages/property/src/__benchmarks__/property-filter-parse.ts

@@ -0,0 +1,41 @@
+import * as Benchmark from "benchmark";
+import { BaseUnits, UnitMap } from "uom";
+import { PropertyFilter } from "../index";
+
+const suite = new Benchmark.Suite();
+
+const unitLookup: UnitMap.UnitLookup = (unitString) => (BaseUnits as UnitMap.UnitMap)[unitString];
+
+const pf = PropertyFilter.fromString(
+  "ccc=20&a=1,2,3~10&d=-50&(ccc=20|a=1,2)&d=5|z=50",
+  unitLookup
+  // "a=1"
+);
+if (!pf) {
+  throw new Error("Could not create property filter.");
+}
+
+suite
+  // add tests
+  .add("PropertyFilter#fromString empty", () => {
+    PropertyFilter.fromString("", unitLookup);
+  })
+  .add("PropertyFilter#fromString whitespace", () => {
+    PropertyFilter.fromString("  ", unitLookup);
+  })
+  .add("PropertyFilter#fromString cache hit", () => {
+    PropertyFilter.fromString("ccc=20&a=1,2,3~10&d=-50&(ccc=20|a=1,2)&d=5|z=50", unitLookup);
+  })
+  .add("PropertyFilter#fromString cache miss", () => {
+    const randomValue = Math.floor(Math.random() * 100000);
+    PropertyFilter.fromString(`ccc=20&a=1,2,3~10&d=-50&(randomvalue=${randomValue}|a=1,2)&d=5|z=50`, unitLookup);
+  })
+  .on("start", () => {
+    console.log("Started benchmark");
+  })
+  // add listeners
+  .on("cycle", (event: Event) => {
+    console.log(String(event.target));
+  })
+  // run async
+  .run({ async: true });

packages/property/src/__tests__/property-filter.test.ts

@@ -48,6 +48,15 @@ describe("PropertyFilter", () => {
       expect(PropertyFilter.equals(filter2, filter1)).toBe(false);
     });
   });
+
+  describe("parse whitespace", () => {
+    it("should parse PropertyFilters with whitespaces and empty strings the same", () => {
+      const filter1 = PropertyFilter.fromStringOrEmpty(" ", unitLookup);
+      const filter2 = PropertyFilter.fromStringOrEmpty("", unitLookup);
+      expect(filter1).toBe(PropertyFilter.Empty);
+      expect(filter2).toBe(PropertyFilter.Empty);
+    });
+  });
 });
 
 function fromStringOrException(filter: string): PropertyFilter.PropertyFilter {

packages/property/src/property-filter.ts

@@ -33,20 +33,29 @@ export function fromString(filter: string, unitLookup: UnitMap.UnitLookup): Prop
   if (filter === null || filter === undefined) {
     throw new Error("Argument 'filter' must be defined.");
   }
-  if (!_cache.has(filter)) {
-    if (filter === "" || filter.trim().length === 0) {
-      return Empty;
-    }
-    const ast = Ast.parse(filter, unitLookup, false);
 
-    if (ast === undefined) {
-      console.warn("Invalid property filter syntax: " + filter);
-      return undefined;
-    }
-    _cache.set(filter, create(filter, ast));
+  if (filter === "") {
+    return Empty;
+  }
+
+  const cachedFilter = _cache.get(filter);
+  if (cachedFilter) {
+    return cachedFilter;
+  }
+
+  if (filter.trim() === "") {
+    return Empty;
+  }
+
+  const ast = Ast.parse(filter, unitLookup, false);
+  if (ast !== undefined) {
+    const parsedFilter = create(filter, ast);
+    _cache.set(filter, parsedFilter);
+    return parsedFilter;
   }
 
-  return _cache.get(filter);
+  console.warn("Invalid property filter syntax: " + filter);
+  return undefined;
 }
 
 export function fromStringOrEmpty(filterString: string, unitLookup: UnitMap.UnitLookup): PropertyFilter {