Skip to content
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

Improve performance by replacing _build_discontinuous_regions() #10225

Merged
merged 4 commits into from Jun 24, 2020

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Jun 23, 2020

Patches._build_discontinuous_regions() is very inefficient, especially memory utilization-wise, due to high GC churn. It's more efficient to just iterate over NaN separated regions and use TypedArray.subarray() to create data views for other functions to use. Additionally I also merged scenterx() and scentery() into one method, as those often had repeated logic and required computing things twice.

}

scentery(i: number): number {
return this._scenterxy(i).y
Copy link
Member

@bryevdv bryevdv Jun 24, 2020

Choose a reason for hiding this comment

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

We should do some investigation of our own to make sure Dask, etc are not relying on these. Or leave them in with a deprecation and a note in the migration guide.

@mattpap mattpap changed the base branch from master to branch-2.2 Jun 24, 2020
@mattpap mattpap force-pushed the mattpap/discontinuous branch from 1ab13ca to 714be18 Compare Jun 24, 2020
@mattpap mattpap force-pushed the mattpap/discontinuous branch from d0be49a to 5482db7 Compare Jun 24, 2020
@mattpap mattpap merged commit 6e3819d into branch-2.2 Jun 24, 2020
@mattpap mattpap deleted the mattpap/discontinuous branch Jun 24, 2020
paul-tqh-nguyen pushed a commit to paul-tqh-nguyen/bokeh that referenced this pull request Jul 9, 2020
…h#10225)

* Replace Patches._build_discontinuous_regions()

* Unify scenter{x,y}() -> scenterxy()

* Keep scenter{x,y}() for backwards compatibility

* Add a migration guide
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants