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

Optimization for bokeh comms messages in dynamic mode #822

Merged
merged 4 commits into from Aug 22, 2016
Merged

Conversation

@philippjfr
Copy link
Member

@philippjfr 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
Contributor

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
Author 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
Contributor

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

This comment has been minimized.

@philippjfr

philippjfr Aug 18, 2016
Author 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
Copy link
Member Author

@philippjfr philippjfr commented Aug 22, 2016

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

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Aug 22, 2016

Looks good. Merging.

@jlstevens jlstevens merged commit c91b70b into master Aug 22, 2016
3 of 4 checks passed
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
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants