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

Improved bokeh json patch computation #807

Merged
merged 2 commits into from Aug 15, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Aug 3, 2016

Improved code that tidies up bokeh json patches to update plots. Should result in an overall reduction of 30-50% in the size of the json containing the plot updates.

Things to do:

  • Test on a wide range of plots to ensure nothing breaks

I've now tested this on every bokeh based notebook I could find and have not encountered any issues. Very happy I could condense the plot updates this much!

Improved bokeh json patch computation
Improved code that tidies up bokeh json patches to update plots.
Should result in an overall reduction of 30-50% in the json size
@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 15, 2016

This is ready to be merged now, I've been running off this branch for almost two weeks now without any issues and it really does represent a huge saving in the amount of data that is sent to the browser.

"""
Returns a list of all ids in the supplied object.
Useful for determining if a json representation contains
references to other objects.

This comment has been minimized.

@jlstevens

jlstevens Aug 15, 2016

Member

Might be worth mentioning why this knowing if the JSON has references is useful information (if it can be summarized in a simple way).

This comment has been minimized.

@philippjfr

philippjfr Aug 15, 2016

Member

Will do.

new_obj[k] = ref
else:
new_obj[k] = v
if k in ['data', 'palette'] or (k == 'attributes' and not get_ids(v)):

This comment has been minimized.

@jlstevens

jlstevens Aug 15, 2016

Member

Will this list of hardcoded strings potentially change? What do they correspond to? It might be worth specifying them as an optional argument (tuple).

This comment has been minimized.

@philippjfr

philippjfr Aug 15, 2016

Member

Agreed, I don't envision them changing but breaking them out probably makes sense regardless. As a quick explanation, there are two bits of data that are sent to update a plot:

  1. The events: These contain the actual attributes that have changed and need to be updated.
  2. The references: These contain references to the objects (using ids) required to correctly update the object graph on the JS side, but also duplicate the data.

This PR filters filters the various data attributes in the references leaving only the id references that are actually needed (which gives the 30-50% reduction in the json payload). This means that the data, the palette and all attributes that do not contain references to other objects are deleted. Passing in the list of attributes that should be dropped explicitly makes sense so I'll do that now.

'FixedTicker', 'FuncTickFormatter', 'LogTickFormatter']
# Where to look for the ignored models
LOCATIONS = ['new', 'below', 'right', 'left',

This comment has been minimized.

@jlstevens

jlstevens Aug 15, 2016

Member

Good to get rid of this!

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 15, 2016

Ok, hopefully two weeks worth of testing means it is robust!

I am happy to merge once you've decided to either ignore/implement the two suggestions I've made in the comments.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 15, 2016

Just implemented your suggestions and improved various docstrings to make it a bit clearer how this stuff actually works. Ready to merge now.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 15, 2016

Looks good! Merging.

@jlstevens jlstevens merged commit be9c6f4 into master Aug 15, 2016

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens removed the in progress label Aug 15, 2016

@jbednar jbednar deleted the bokeh_json_opt branch Aug 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment