This repository has been archived by the owner on Apr 25, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 12
perf: add cache support to evaluation in reearth/core #573
Open
pyshx
wants to merge
19
commits into
main
Choose a base branch
from
perf/style-evaluation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
224f35e
perf: add worker
pyshx 9e469d3
perf: add concurrency and cache support in evaluation
pyshx 521c595
perf: replace normal cache with LRU
pyshx fcb28ba
Merge branch 'main' into perf/style-evaluation
pyshx 3d7c212
ci: remove worker API related test
pyshx 6c2b4db
Merge branch 'perf/style-evaluation' of github.com:reearth/reearth-we…
pyshx 2f094f5
chore: refactor
pyshx 672c1a9
fix: remove worker support and handle jsonpath for evalExp cache
pyshx 5f779d8
chore: merge conflict resolve
pyshx 9a936d1
Merge branch 'main' into perf/style-evaluation
pyshx 1bd2502
fix: handle jsonPath properly for cache key generation
pyshx 5e0ff32
Merge branch 'perf/style-evaluation' of github.com:reearth/reearth-we…
pyshx 83ae0ee
Merge branch 'main' into perf/style-evaluation
pyshx fbdd936
refactor
pyshx dff623c
Merge branch 'perf/style-evaluation' of github.com:reearth/reearth-we…
pyshx 09789b7
fix: add cache for getReference results in utils
pyshx 8e9298a
fix: onclick hightlight
pyshx 64182b1
ci fix
pyshx d4b953f
Merge branch 'main' into perf/style-evaluation
keiya01 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
import { expect, test, describe } from "vitest"; | ||
|
||
import { ConditionsExpression } from "../../types"; | ||
|
||
import { getReferences, getCacheableProperties, getCombinedReferences } from "./utils"; | ||
|
||
describe("getCacheableProperties", () => { | ||
const feature = { foo: "bar", baz: "qux" }; | ||
const styleExpression: ConditionsExpression = { | ||
conditions: [ | ||
["${foo}", "${baz}"], | ||
["${qux}", "121"], | ||
], | ||
}; | ||
|
||
test("should return properties that exist in feature and are referenced in styleExpression", () => { | ||
const result = getCacheableProperties(styleExpression, feature); | ||
expect(result).toEqual({ foo: "bar", baz: "qux" }); | ||
}); | ||
|
||
test("should return an empty object if no properties are referenced in styleExpression", () => { | ||
const styleExpression = "some string"; | ||
const result = getCacheableProperties(styleExpression, feature); | ||
expect(result).toEqual({}); | ||
}); | ||
}); | ||
|
||
describe("getCombinedReferences", () => { | ||
const feature = { foo: "bar", baz: "qux" }; | ||
|
||
test("should return references in a single expression", () => { | ||
const expression = "${foo}"; | ||
const result = getCombinedReferences(expression, feature); | ||
expect(result).toEqual(["foo"]); | ||
}); | ||
|
||
test("should return references in a style expression with multiple conditions", () => { | ||
const expression: ConditionsExpression = { | ||
conditions: [ | ||
["${foo}", "${baz}"], | ||
["${qux}", "121"], | ||
], | ||
}; | ||
const result = getCombinedReferences(expression, feature); | ||
expect(result).toEqual(["foo", "baz", "qux"]); | ||
}); | ||
|
||
test("should return an empty array if expression is a string with no references", () => { | ||
const expression = "some string"; | ||
const result = getCombinedReferences(expression, feature); | ||
expect(result).toEqual([]); | ||
}); | ||
}); | ||
|
||
describe("getReferences", () => { | ||
const feature = { | ||
foo: "bar", | ||
baz: "qux", | ||
obj: { | ||
arr: [1, 2, 3], | ||
nestedObj: { | ||
a: "A", | ||
b: "B", | ||
}, | ||
}, | ||
}; | ||
|
||
test("should return references in a single expression", () => { | ||
const expression = "${foo}"; | ||
const result = getReferences(expression, feature); | ||
expect(result).toEqual(["foo"]); | ||
}); | ||
|
||
test("should return references in an expression with JSONPath expressions", () => { | ||
const expression = "${$.baz}"; | ||
const result = getReferences(expression, feature); | ||
expect(result).toEqual(['["baz"]']); | ||
}); | ||
|
||
test("should return references for nested JSONPath expressions", () => { | ||
const expression = "${$.obj.nestedObj.a}"; | ||
const result = getReferences(expression, feature); | ||
expect(result).toEqual(['["obj"]["nestedObj"]["a"]']); | ||
}); | ||
|
||
test("should handle JSONPath expressions that return arrays", () => { | ||
const expression = "${$.obj.arr[1]}"; | ||
const result = getReferences(expression, feature); | ||
expect(result).toEqual(['["obj"]["arr"]["1"]']); | ||
}); | ||
|
||
test("should return an empty array if expression has no references", () => { | ||
const expression = "some string"; | ||
const result = getReferences(expression, feature); | ||
expect(result).toEqual([]); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
import { JSONPath } from "jsonpath-plus"; | ||
import { pick } from "lodash-es"; | ||
|
||
import { StyleExpression } from "../../types"; | ||
|
||
export function getCacheableProperties(styleExpression: StyleExpression, feature?: any) { | ||
const properties = pick(feature, getCombinedReferences(styleExpression, feature)); | ||
return properties; | ||
} | ||
|
||
export function getCombinedReferences(expression: StyleExpression, feature?: any): string[] { | ||
if (typeof expression === "string") { | ||
return getReferences(expression, feature); | ||
} else { | ||
const references: string[] = []; | ||
for (const [condition, value] of expression.conditions) { | ||
references.push(...getReferences(condition, feature), ...getReferences(value, feature)); | ||
} | ||
return references; | ||
} | ||
} | ||
|
||
export function getReferences(expression: string, feature?: any): string[] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have two function to parse expression. First is this function. Second is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'll be hard to separate out alot the repetitive logic and avoid iterating over the string again in |
||
const result: string[] = []; | ||
let exp = expression; | ||
let i = exp.indexOf("${"); | ||
const varExpRegex = /^\$./; | ||
const jsonPathCache: Record<string, any[]> = {}; | ||
|
||
while (i >= 0) { | ||
const openSingleQuote = exp.indexOf("'", i); | ||
const openDoubleQuote = exp.indexOf('"', i); | ||
|
||
if (openSingleQuote >= 0 && openSingleQuote < i) { | ||
const closeQuote = exp.indexOf("'", openSingleQuote + 1); | ||
result.push(exp.substring(0, closeQuote + 1)); | ||
exp = exp.substring(closeQuote + 1); | ||
} else if (openDoubleQuote >= 0 && openDoubleQuote < i) { | ||
const closeQuote = exp.indexOf('"', openDoubleQuote + 1); | ||
result.push(exp.substring(0, closeQuote + 1)); | ||
exp = exp.substring(closeQuote + 1); | ||
} else { | ||
const j = exp.indexOf("}", i); | ||
|
||
if (j < 0) { | ||
console.log("Unmatched {."); | ||
} | ||
pyshx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const varExp = exp.slice(i + 2, j); | ||
if (varExpRegex.test(varExp)) { | ||
let res = jsonPathCache[varExp]; | ||
if (!res) { | ||
try { | ||
res = JSONPath({ json: feature, path: varExp }); | ||
jsonPathCache[varExp] = res; | ||
} catch (error) { | ||
console.log("Invalid JSONPath"); | ||
pyshx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
if (res.length !== 0) { | ||
console.log("JSONPathEval: ", res[0]); | ||
pyshx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const keyPath = getObjectPathByValue(feature, res[0]); | ||
pyshx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (keyPath) result.push(keyPath); | ||
else return Object.keys(feature); | ||
} | ||
} else { | ||
result.push(exp.substring(i + 2, j)); | ||
} | ||
exp = exp.substring(j + 1); | ||
} | ||
|
||
i = exp.indexOf("${"); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
function getObjectPathByValue(obj: any, value: any): string | undefined { | ||
for (const key in obj) { | ||
if (Object.prototype.hasOwnProperty.call(obj, key)) { | ||
pyshx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const prop = obj[key]; | ||
if (prop === value) { | ||
return `[${JSON.stringify(key)}]`; | ||
} else if (typeof prop === "object") { | ||
const nestedKey = getObjectPathByValue(prop, value); | ||
if (nestedKey !== undefined) { | ||
return `[${JSON.stringify(key)}]${nestedKey}`; | ||
} | ||
} | ||
} | ||
} | ||
return undefined; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to instantiate Expression no more, but we become to need to parse expression every time...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this will be less heavy with the cache for getReferences in place.