-
Notifications
You must be signed in to change notification settings - Fork 10
chore(): cache cleanup #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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.
stable and ready for review
| (pth.arr = ( | ||
|
|
||
| // rough assumption | ||
| (!isArray(path) || !isArray(path && path[0]))) ? |
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.
I don't understand this...
| (!isArray(path) || !isArray(path && path[0]))) ? | |
| typeof path === 'string' ? |
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.
|
The current CI failure is not related to your PR. Let me fix that... |
Problem will be fixed via #28 |
|
add prettierrc to make my life easier but still there is whitespace mayham |
Please do not do that. Keep changes minimal, easy to follow. |
|
I believe I kept changes minimal as possible. |
|
I expected lint to fix whitespace but it doesn't |
d63f633 to
a773a2e
Compare
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.
expose parsePathCurve
c01af55 to
9cfa863
Compare
|
@ShaMan123 CI is failing. Please ensure |
|
You could add a test case to verify caching (intersecting between two cached paths should be faster than intersecting between two uncached paths). |
I am not sure if the test is reliable enough, but it is added |
|
Fixed failing TS and added a test to verify |
a7cd913 to
6c39a63
Compare
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.
added a cache timing test but I do not recommend keeping it. It is flaky and IMO gains nothing.
6c39a63 to
6478713
Compare
6478713 to
6772ff5
Compare
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.
Revisited the caching perf test to make it more robust and found out that for 100/1000 path segments the difference in runtime in 3ms. For 2 segments caching was degrading performance.
All in all seems that for most cases the code's caching is degrading or not boosting enough to make it worthwhile.
With this PR a consumer can parse and cache paths so I would remove caching in the next major release.
Proposed Changes
Related to #26, preliminary work
Closes #25
Indexing paths means the index needs to hold the path in memory, making caching redundant.
So I wish to unblock myself before moving forward.
I have encapsulated all caching logic into a single method
getPathCurve.This is a main entry point that will not be used by the indexing method.
This is where path parsing is handled as well.
I don't really understand why there are 3 levels of caching (curve, abs, arr) and I find the caching engine a bit weird (does it rm an entry after 100 invocations?) but that is off topic and will be made redundant once indexing is used.
Additionally I took the opportunity to inspect
pathCloneusages. They were called excessively so I have removed those. The place that mutates path data ispathToCurve.Then I looked into the implementation and decided to simplify it as well (for perf, readability and code quality).
I exposed
parsePathCurveso a consumer can opt out of internal path caching.Finally, I've extracted
isBBoxIntersectearly return up to its caller since the spatial index will be handling bbox intersection logic.Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}