8000 Fast color parsing by dy · Pull Request #1443 · plotly/plotly.js · GitHub
[go: up one dir, main page]
Skip to content

Fast color parsing #1443

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and p 8000 rivacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Mar 14, 2017
Merged

Fast color parsing #1443

merged 11 commits into from
Mar 14, 2017

Conversation

dy
Copy link
Contributor
@dy dy commented Mar 3, 2017

Implement faster color parsing for the gl-scatter2d-sdf module.

etpinard
etpinard reviewed Mar 3, 2017
View reviewed changes
src/lib/gl_format_color.js
var isNumeric = require('fast-isnumeric');
var rgba = require('color-rgba');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add color-rgba to the package.json and push again?

I'll be interesting to see if the tests pass on CI ...

@dy
Copy link
Contributor Author
dy commented Mar 3, 2017 •
edited
Loading

@etpinard whoops, done.
Perf boost is noticeable only for large data sets in gl-scatter2d-sdf.
As for existing ~2s plotly.js lag the bottleneck is something different.

@dy
Copy link
Contributor Author
dy commented Mar 3, 2017

@etpinard ok, it speeds up color parsing compared to the previous one, but in total it seems to be not the biggest bottleneck.

@etpinard
Copy link
Contributor
etpinard commented Mar 3, 2017

@dfcreative any perf improvements are always welcome 🐎

Look like one 3D mock is failing though

image

@dy
Copy link
Contributor Author
dy commented Mar 3, 2017 •
edited
Loading

@etpinard fixed that case. Turned out some marker colors are (mistakenly) parsed from numbers, and color-parse treats numbers as possible color values.

@etpinard etpinard added this to the v1.25.0 milestone Mar 6, 2017
@etpinard
Copy link
Contributor
etpinard commented Mar 13, 2017 •
edited
Loading

Looks like commit 22eacec broke a few things.

@dfcreative do you think you'll have the time to make the tests go ✅ again in the nex 8000 t few days? If not, I'll drop it from the 1.25.0 release, no big deal. If you do have the time to look at this, make sure to merge the latest master into this branch to make CI happy post-#1455.

@dy
Copy link
Contributor Author
dy commented Mar 13, 2017

@etpinard yes, sure, I'll give it a shot tonight.

@dy
Copy link
Contributor Author
dy commented Mar 14, 2017

@etpinard may I ask you to run baselines to show list of broken ones? I am having a hard time getting info from circleci. Would appreciate

@etpinard
Copy link
Contributor

Sure. Merging master into this branch should help though.

@dy
Copy link
Contributor Author
dy commented Mar 14, 2017

@etpinard there are two failed tests, looks like they are not related to colors. Should I try to reproduce them or it is transitional thing?

@etpinard
Copy link
Contributor
etpinard commented Mar 14, 2017 •
edited
Loading

Oh shit. Yeah I've seen those tests fail intermittently. Don't worry about them.

Thanks again for this PR 🎉

@etpinard 9962
Copy link
Contributor

Sweet. Tests are ✅ again.

Looks like the broken tests I mentioned in #1443 (comment) where simply due to uglifyjs@2.8.x causing havoc as described in #1450 (comment). Commit 22eacec didn't actually break anything.

Thanks @dfcreative !!

@etpinard etpinard merged commit 9b654dd into plotly:master Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etpinard etpinard etpinard left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.25.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dy @etpinard
0