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

Optimization for bokeh comms messages in dynamic mode #822

Merged
merged 4 commits into from Aug 22, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Aug 18, 2016

Very simple optimizations for dynamic mode, including caching of the previous frame to check whether it is unchanged. Happy to hear a cleaner approach to caching the previous frame, but it's probably not worth factoring out that logic into some other object, since it's so trivial.

@@ -239,7 +239,8 @@ def compute_static_patch(document, models):
update_types[obj['type']].append(key)
events = [delete_refs(e, IGNORED_MODELS)
for _, e in sorted(events, key=lambda x: x[0])]
events = [e for e in events if all(i in requested_updates for i in get_ids(e))]
events = [e for e in events if all(i in requested_updates for i in get_ids(e))
if 'new' in e]

This comment has been minimized.

@jlstevens

jlstevens Aug 18, 2016

Member

Looks like anything with the substring 'new' will be included. Even if it is safe, it would be good to at least have a comment justifying this condition.

This comment has been minimized.

@philippjfr

philippjfr Aug 18, 2016

Member

Will do. The events are dictionaries and only events which actually include a new value need to be sent.

This comment has been minimized.

@jlstevens

jlstevens Aug 18, 2016

Member

And 'new' is in there as a string? Ok!

This comment has been minimized.

@philippjfr

philippjfr Aug 18, 2016

Member

'new' is the key containing the updated value. Not sure why there are events without a 'new' value in the first place since they have no effect anyway.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 22, 2016

PR build passed, push build is failing because the branch is behind master slightly. Ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 22, 2016

Looks good. Merging.

@jlstevens jlstevens merged commit c91b70b into master Aug 22, 2016

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 46.683%
Details
s3-reference-data-cache Test data is cached.
Details

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

@philippjfr philippjfr deleted the dynamic_opt branch Sep 2, 2016

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