-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Lens] Create mathColumn function to improve performance #101908
[Lens] Create mathColumn function to improve performance #101908
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
Pinging @elastic/kibana-app (Team:KibanaApp) |
throw new Error('ID must be unique'); | ||
} | ||
|
||
const newRows = input.rows.map((row) => { |
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.
It would be nice to have this processed in async chunks, in order to give the thread some time to run some small tasks here and there if very big tables are passed.
Lodash exposes a chunks
utility for this. What do you think?
: [], | ||
}, | ||
], | ||
expression: [currentColumn.references.length ? `"${currentColumn.references[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.
Seems like this is causing the failing test with an empty formula which is annoying - I tried to fix it using staticColumn
, but it's missing separate id/name params. We could add those, not sure whether there's a more elegant solution.
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 best solution I've found is to use mapColumn
with an empty expression.
const newRows = input.rows.map((row) => { | ||
return { | ||
...row, | ||
[args.id]: math.fn( |
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.
This is still calling math
separately for each row which causes the tinymath parser to run many times. If there are a lot of rows, this becomes relevant to performance (~4k rows with a very simple formula - can get worse when multiple math contexts are used for column wise calculations):
I propose we cache the ast by not calling evaluate
, but parse
, then interpret
. This can be done either by pulling the math logic into this expression function so we can simply call parse
once, then interpret
for every row, or by using memoize-one in the math function on the parse
call.
Can be done in a separate PR.
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 have added the memoization to tinymath in this PR as it definitely improves the overall speed.
{ | ||
expression: args.expression, | ||
onError: args.onError, | ||
}, |
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.
This object could be declared on top and reused over and over. Just saving some memory.
I've made also an experiment reusing the same table "template" above, but in terms of performance results were negligible for a medium size table, so not worth the hack.
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.
Unsure about the memoization of the parsing, could you check?
packages/kbn-tinymath/src/index.js
Outdated
@@ -23,7 +24,7 @@ function parse(input, options) { | |||
} | |||
|
|||
try { | |||
return parseFn(input, options); | |||
return memoizeOne(parseFn)(input, options); |
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.
Didn't test (I can do tomorrow), but is this actually memoizing? Looking at the memoize-one source code, it seems like memoizeOne
itself is not memoized on the passed-in function so it would create a new memoization closure on each call without actually ever hitting the cache.
Looks like the memoizeOne
call should be moved outside of the parse
function
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.
You're totally right, the memoizeOne function returns a instance each time!
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
code LGTM
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.
Tested and parse time as well as overall time spent with doing math went down significantly, LGTM.
There are still optimizations we can do e.g. not having "pass through" mathColumn
calls on the root and for things like moving_average(count())
(right now there's a math call to copy the count metric, then the moving_average call, then a math call for copying the moving average result into the final column - we only need the moving_average call). But let's do those separately
) * [Lens] Create mathColumn function to improve performance * Fix empty formula case * Fix tinymath memoization Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…102356) * [Lens] Create mathColumn function to improve performance * Fix empty formula case * Fix tinymath memoization Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Wylie Conlon <[email protected]>
@wylieconlon This PR has broken Canvas Storybook... investigating why with @spalger. |
We have a fix-- it originated in the |
) * [Lens] Create mathColumn function to improve performance * Fix empty formula case * Fix tinymath memoization Co-authored-by: Kibana Machine <[email protected]>
This is one of the followups needed to improve Lens formula performance. We found unacceptably slow performance when using
mapColumn
+math
in combination, where the total execution time for common cases was several seconds long. By combining this into a singlemathColumn
function we are able to get consistent performance.Checklist
Delete any items that are not applicable to this PR.