-
Notifications
You must be signed in to change notification settings - Fork 1
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
Low-hanging perf improvements in highlighting code #196
Conversation
🦋 Changeset detectedLatest commit: 462eaf0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
3554846
to
21436b6
Compare
21436b6
to
1237b3a
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.
It may be worth looking at some of these functions/flows to determine if a simple for
loop could be much more performant.
(acc, field, path) => { | ||
let subFields = getFieldSubFields(field) | ||
for (let k in subFields) acc[`${path}.${k}`] = subFields[k] | ||
return acc |
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.
Mutating the accumulator and then returning it feels kind of silly to me. Why not just use a for
loop. It's easy to read and clearly indicates the imperative approach you're taking here.
@@ -6,7 +6,7 @@ import { groupStats } from './groupStatUtils.js' | |||
// [1, 2, 3] -> [{to: 1}, {from: 1, to: 2}, {from: 2, to: 3}, {from: 3}] | |||
let boundariesToRanges = _.flow( | |||
F.mapIndexed((to, i, list) => F.compactObject({ from: list[i - 1], to })), | |||
(arr) => F.push({ from: _.last(arr).to }, arr) | |||
(arr) => F.pushOn(arr, { from: _.last(arr).to }) |
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 think I actually prefer F.push
here. Mixing non-mutating with mutating in the same flow is kind of awkward for me. If you want to mutate an array with push
you should just loop over the array and do that.
@@ -148,7 +151,7 @@ export let getAllHighlightFields = _.memoize((schema) => { | |||
let collectKeysAndValues = (f, coll) => | |||
F.reduceTree()( | |||
(acc, val, key) => | |||
f(val) ? F.push(val, acc) : f(key) ? F.push(key, acc) : acc, | |||
f(val) ? F.pushOn(acc, val) : f(key) ? F.pushOn(acc, key) : acc, |
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.
Same kind of thing here. Using reduce
with a mutating call in the reducing function feels a little awkward to me. Why not just walk the tree and and push
onto an array?
1d609cc
to
462eaf0
Compare
Coverage after merging feature/highlighting-perf-improvements into main will be
Coverage Report
|
Use
F.pushOn
in more places and a few perf tweaks in hot code paths.