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

Various fixes for bokeh static_source optimization #1413

Merged
merged 5 commits into from May 28, 2017
Merged
Diff settings

Always

Just for now

@@ -18,6 +18,7 @@
from .options import Store, StoreOptions
from .pprint import PrettyPrinter

obj_id = id
# Alias parameter support for pickle loading

ALIASES = {'key_dimensions': 'kdims', 'value_dimensions': 'vdims',
@@ -479,7 +480,7 @@ class LabelledData(param.Parameterized):

_deep_indexable = False

def __init__(self, data, id=None, **params):
def __init__(self, data, id=None, plot_id=None, **params):
"""
All LabelledData subclasses must supply data to the
constructor, which will be held on the .data attribute.
@@ -488,6 +489,7 @@ def __init__(self, data, id=None, **params):
"""
self.data = data
self.id = id

This comment has been minimized.

Copy link
@jlstevens

jlstevens May 28, 2017

Contributor

Do we ever actually set the id via the constructor?

This comment has been minimized.

Copy link
@jlstevens

jlstevens May 28, 2017

Contributor

Ah, probably for clone. Nevermind my question!

This comment has been minimized.

Copy link
@philippjfr

philippjfr May 28, 2017

Author Contributor

Yes, clone does all the time.

self._plot_id = plot_id or obj_id(self)

This comment has been minimized.

Copy link
@jlstevens

jlstevens May 28, 2017

Contributor

Just use __builtins__.id to avoid the name shadowing issue.

if isinstance(params.get('label',None), tuple):
(alias, long_name) = params['label']
label_sanitizer.add_aliases(**{alias:long_name})
@@ -531,6 +533,7 @@ def clone(self, data=None, shared_data=True, new_type=None, *args, **overrides):

if data is None and shared_data:
data = self.data
settings['plot_id'] = self._plot_id
# Apply name mangling for __ attribute
pos_args = getattr(self, '_' + type(self).__name__ + '__pos_params', [])
return clone_type(data, *args, **{k:v for k,v in settings.items()
@@ -363,7 +363,7 @@ def __init__(self, items=None, identifier=None, parent=None, **kwargs):
self.__dict__['_max_cols'] = 4
if items and all(isinstance(item, Dimensioned) for item in items):
items = self._process_items(items)
params = {p: kwargs.pop(p) for p in list(self.params().keys())+['id'] if p in kwargs}
params = {p: kwargs.pop(p) for p in list(self.params().keys())+['id', 'plot_id'] if p in kwargs}
AttrTree.__init__(self, items, identifier, parent, **kwargs)
Dimensioned.__init__(self, self.data, **params)

@@ -766,6 +766,7 @@ def clone(self, data=None, shared_data=True, new_type=None, *args, **overrides):

if data is None and shared_data:
data = self.data
settings['plot_id'] = self._plot_id
# Apply name mangling for __ attribute
pos_args = getattr(self, '_' + type(self).__name__ + '__pos_params', [])
with item_check(not shared_data and self._check_items):
@@ -856,6 +856,7 @@ def clone(self, data=None, shared_data=True, new_type=None, link_inputs=True,
"""
if data is None and shared_data:
data = self.data
overrides['plot_id'] = self._plot_id
clone = super(UniformNdMapping, self).clone(overrides.pop('callback', self.callback),
shared_data, new_type,
*(data,) + args, **overrides)
@@ -146,6 +146,8 @@ def clone(self, *args, **overrides):
containing the specified args and kwargs.
"""
settings = dict(self.get_param_values(), **overrides)
if not args:
settings['plot_id'] = self._plot_id
return self.__class__(*args, **settings)


@@ -804,15 +804,16 @@ def update_frame(self, key, ranges=None, plot=None, element=None, empty=False):
# Cache frame object id to skip updating data if unchanged
previous_id = self.handles.get('previous_id', None)
if self.batched:
current_id = sum(element.traverse(lambda x: id(x.data), [Element]))
current_id = sum(element.traverse(lambda x: x._plot_id, [Element]))
else:
current_id = id(element.data)
current_id = element._plot_id

This comment has been minimized.

Copy link
@jlstevens

jlstevens May 28, 2017

Contributor

Is it no longer taking into account the id of the data? It seems to do that in tabular...

self.handles['previous_id'] = current_id
self.static_source = self.dynamic and (current_id == previous_id)
self.static_source = (self.dynamic and (current_id == previous_id))
if self.batched:
data, mapping = self.get_batched_data(element, ranges, empty)
else:
data, mapping = self.get_data(element, ranges, empty)

if not self.static_source:
self._update_datasource(source, data)

@@ -147,7 +147,7 @@ def sync_sources(self):
from the same object.
"""
get_sources = lambda x: (id(x.current_frame.data), x)
filter_fn = lambda x: (x.shared_datasource and x.current_frame and
filter_fn = lambda x: (x.shared_datasource and x.current_frame is not None and

This comment has been minimized.

Copy link
@jlstevens

jlstevens May 28, 2017

Contributor

Just to confirm: is this a a fix unrelated to the optimization itself?

This comment has been minimized.

Copy link
@philippjfr

philippjfr May 28, 2017

Author Contributor

Seems so, not sure why I pushed it here, but it is an important fix.

not isinstance(x.current_frame.data, np.ndarray)
and 'source' in x.handles)
data_sources = self.traverse(get_sources, [filter_fn])
@@ -73,11 +73,12 @@ def current_handles(self):
if self.static and not self.dynamic:
return handles


element = self.current_frame
previous_id = self.handles.get('previous_id', None)
current_id = id(self.current_frame.data) if self.current_frame else None
current_id = None if self.current_frame is None else id(element)+id(element.data)

This comment has been minimized.

Copy link
@jlstevens

jlstevens May 28, 2017

Contributor

Summing ids is not a safe test for uniqueness as many pairs of ids could sum to the same value (even if it is unlikely!). Note that collisions might not even be that rare as the id doesn't return a hash - it is more like to a memory address.

To check the identity of both the element and the data I would use the tuple (id(element), id(element.data)) and not the sum.

for handle in self._update_handles:
if (handle == 'source' and self.dynamic and
current_id == previous_id):
if (handle == 'source' and self.dynamic and current_id == previous_id):
continue
if handle in self.handles:
handles.append(self.handles[handle])
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.