Skip to content

Remove the performance overhead of showLink property - #1557

Merged
etpinard merged 6 commits into
plotly:masterfrom
hy9be:perf-addLinks
Apr 7, 2017
Merged

Remove the performance overhead of showLink property#1557
etpinard merged 6 commits into
plotly:masterfrom
hy9be:perf-addLinks

Conversation

@hy9be

@hy9behy9be commented Apr 5, 2017

Copy link
Copy Markdown
Contributor

A fix for: #1554. Do nothing if showLink is set to be false, to avoid performance overhead caused by measuring and reflows.

Comment threadsrc/plots/plots.js Outdated
* Add source links to your graph inside the 'showSources' config argument.
*/
plots.addLinks = function(gd) {
if(gd._context.showLink === false) return;

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.

This could break graphs that have the showSources config option set as it is called below.

So, replacing this line with something like:

if(!gd._context.showLink||!gd._context.showSources)return;

would be safer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Got it. Thanks!

Made changes according to code review feedback.
@hy9be

hy9be commented Apr 6, 2017

Copy link
Copy Markdown
ContributorAuthor

Code updated according to the review feedback.

@etpinard

Copy link
Copy Markdown
Contributor

Unfortunately, your latest commit made npm run test-jasmine -- config fail:

image

@hy9be

hy9be commented Apr 6, 2017

Copy link
Copy Markdown
ContributorAuthor

Em, Interesting...I will take a look. Did not run the test locally as I thought it is just a trivial change.

@hy9be

hy9be commented Apr 6, 2017

Copy link
Copy Markdown
ContributorAuthor

I changed the OR condition into AND:

if(!gd._context.showLink&&!gd._context.showSources)return;

It seems to make sense to me logically, and all tests pass. But please review. Thanks!

@hy9be

hy9be commented Apr 6, 2017

Copy link
Copy Markdown
ContributorAuthor

I cannot reproduce the errors reported by CI. And it does not have information about test cases and lines. Any suggestion on debugging?

image

@etpinard

Copy link
Copy Markdown
Contributor

CI uses an older version of Chrome (54, the latest is 57 I think), so maybe that's the issue.

Otherwise, have you tried running all the test suites at once with npm run jasmine? Maybe on test case isn't cleaning up correctly?

@hy9be

hy9be commented Apr 6, 2017

Copy link
Copy Markdown
ContributorAuthor

The error doesn't look like related to browser version.

Yes I run the full jasmine test. And only two noCI cases failed:

image
image

@hy9behy9be closed this Apr 7, 2017
@hy9behy9be reopened this Apr 7, 2017
@hy9be

hy9be commented Apr 7, 2017

Copy link
Copy Markdown
ContributorAuthor

Run all the test cases locally again. All 1850 cases passed this time. So I add some comments to re-trigger the CI check.

@etpinard

Copy link
Copy Markdown
Contributor

@hy9be tests are passing. Looks like the CI failure was intermittent. Our apologies.

Great PR. Thanks againg 🎉

Merging.

@etpinard
etpinard merged commit 9e9d756 into plotly:masterApr 7, 2017
@hy9behy9be mentioned this pull request Apr 11, 2017
Sign up for freeto 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.

2 participants

@hy9be@etpinard