FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[perf] cache resolved data element options by kurkle · Pull Request #6579 · chartjs/Chart.js · GitHub

[perf] cache resolved data element options - #6579

Merged
etimberg merged 3 commits into
chartjs:masterfrom
kurkle:option-caching
Oct 19, 2019
Merged

[perf] cache resolved data element options#6579
etimberg merged 3 commits into
chartjs:masterfrom
kurkle:option-caching

Conversation

kurkle commented Oct 18, 2019
edited
Loading

Copy link
Copy Markdown
Member

Alternative approach that came to mind while taking a look at #6574

Closes #6382

Comment thread src/helpers/helpers.options.js Outdated
* @param {object} [context] - If defined and the current value is a function, the value
* is called with `context` as first argument and the result becomes the new input.
* @param {number} [index] - If defined and the current value is an array, the value
* @param {object} [info] - object to return information about resolution in

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think I'd initialize info.cacheable to true in this method so that if you call resolve outside of _resolveDataElementOptions then info contains a usable value. Then in _resolveDataElementOptions you can check if (info.cacheable && !custom) instead of if (info.cacheable)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It would have to be something like

if (info && info.cacheable === undefined) {
  info.cacheable = true;
}

because resolve is called in a loop. But I think its not needed. You'd need to provide the info object anyway, so not a biggie to provide initial value too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In other words, I like to keep the initialization (and any related if's) out of loop

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe just document it then?

* @param {boolean} [info.cacheable] - Should be initialized to true. Will be updated by this method.

Comment thread src/helpers/helpers.options.js Outdated
Comment thread src/core/core.datasetController.js Outdated

benmccann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks good! Definitely less code than my version!

I had opened my PR previously and you left a comment that I didn't understand. I was going to follow up with you to clarify, but now I understand :-) Thanks so much!

benmccann previously approved these changes Oct 18, 2019

Copy link
Copy Markdown
Member

Was about to merge but I thought of this case:

  1. User creates a chart with 1 dataset and sets its borderColor to red. We cache the options
  2. User changes the borderColor to blue and calls chart.update()
  3. The cache is never invalidated so the chart does not update

Or am I missing something here?

Copy link
Copy Markdown
Contributor

DatasetController does me._cachedDataOpts = null; in _update. Won't that invalidate the cache?

Copy link
Copy Markdown
Member

Right, just wanted to make sure that case was tested

etimberg merged commit 6bc6630 into chartjs:master Oct 19, 2019
kurkle deleted the option-caching branch November 13, 2019 06:12
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* [perf] cache resolved data element options
* Address review comments
* Move uninitialized variables, update comments
This file contains hidden or 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

performance regression in 2.8.0

3 participants


Back | FazBrowse Home | New Git URL