Skip to content

Commit

Permalink
Improve performance of graph changes (#6954)
Browse files Browse the repository at this point in the history
Fixes performance problems observed when creating/resolving errors (#6674):

|before|after|
|---|---|
|![vokoscreenNG-2023-06-09_08-49-46.webm](https://github.com/enso-org/enso/assets/1047859/a0048b32-4906-41cd-8899-6e2543ef6942)|![vokoscreenNG-2023-06-09_08-50-54.webm](https://github.com/enso-org/enso/assets/1047859/fef81512-ad89-4418-ae10-d54de94d96ea)|

This also helps with #6637, although I haven't been able to reproduce the degree of slowness shown there so I can't confirm that this resolves that issue.

# Important Notes
- Disable visualizations until shown. [Faster startup, and all graph changes.]
- 6x faster message deserialization. [Saves 400ms when making a change with many visualizations open.]
- Fast edge recoloring. [Saves 100-150ms when disconnecting an edge in Orders.]
- Add a checked implementation of a `profiler` data structure, used instead of the fast `unsafe` version when `debug-assertions` are enabled.
  • Loading branch information
kazcw authored Jun 14, 2023
1 parent 1fdad39 commit 5de0e6d
Show file tree
Hide file tree
Showing 23 changed files with 365 additions and 238 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ futures = { version = "0.3" }
itertools = { version = "0.10.5" }
lazy_static = { version = "1.4" }
paste = { version = "1.0" }
serde_json = { version = "1.0" }
serde_json = { version = "1.0", features = ["raw_value"] }
smallvec = { version = "1.0.0" }
weak-table = { version = "0.3.0" }
gen-iter = { version = "0.2.1" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ impl Path {
// ====================

/// Notification generated by the Language Server.
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, IntoStaticStr)]
#[derive(Clone, Debug, Deserialize, IntoStaticStr, Derivative)]
#[derivative(PartialEq)]
#[serde(tag = "method", content = "params")]
pub enum Notification {
/// Filesystem event occurred for a watched path.
Expand All @@ -128,7 +129,7 @@ pub enum Notification {
/// Sent from the server to the client to inform about new information for certain expressions
/// becoming available. This notification is superseded by executionContext/expressionUpdates.
#[serde(rename = "executionContext/expressionValuesComputed")]
ExpressionValuesComputed(serde_json::Value),
ExpressionValuesComputed(#[derivative(PartialEq = "ignore")] serde::de::IgnoredAny),

/// Sent from the server to the client to inform about new information for certain expressions
/// becoming available.
Expand Down
1 change: 1 addition & 0 deletions app/gui/src/controller/graph/widget/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use ide_view::graph_editor::ArgumentWidgetConfig;
/// data. Allows for partial deserialization: if any of the widget definitions fails to deserialize,
/// it will be skipped, but the deserialization will continue. All errors are returned as a separate
/// list.
#[profile(Debug)]
pub fn deserialize_widget_definitions(
data: &VisualizationUpdateData,
db: &SuggestionDatabase,
Expand Down
30 changes: 11 additions & 19 deletions app/gui/src/presenter/graph/visualization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,32 @@ use ide_view::graph_editor::component::visualization as visualization_view;
// === Model ===
// =============

#[derive(Clone, CloneRef, Debug)]
#[derive(Debug)]
struct Model {
controller: controller::Visualization,
graph_view: view::graph_editor::GraphEditor,
manager: Rc<Manager>,
error_manager: Rc<Manager>,
state: Rc<graph::state::State>,
shown: RefCell<HashSet<ViewNodeId>>,
}

impl Model {
/// Handle the showing visualization UI.
fn visualization_shown(&self, node_id: ViewNodeId, metadata: visualization_view::Metadata) {
self.shown.borrow_mut().insert(node_id);
self.update_visualization(node_id, &self.manager, Some(metadata));
}

/// Handle the hiding in UI.
fn visualization_hidden(&self, node_id: view::graph_editor::NodeId) {
self.shown.borrow_mut().remove(&node_id);
self.update_visualization(node_id, &self.manager, None);
}

/// Handle the node removal in UI.
fn node_removed(&self, node_id: view::graph_editor::NodeId) {
self.shown.borrow_mut().remove(&node_id);
if self.state.ast_node_id_of_view(node_id).is_some() {
self.update_visualization(node_id, &self.manager, None);
self.update_visualization(node_id, &self.error_manager, None);
Expand All @@ -56,8 +60,10 @@ impl Model {
node_id: ViewNodeId,
preprocessor: visualization_view::instance::PreprocessorConfiguration,
) {
let metadata = visualization_view::Metadata { preprocessor };
self.update_visualization(node_id, &self.manager, Some(metadata))
if self.shown.borrow().contains(&node_id) {
let metadata = visualization_view::Metadata { preprocessor };
self.update_visualization(node_id, &self.manager, Some(metadata))
}
}

/// Handle the error change on given node: attach/detach the error visualization if needed.
Expand Down Expand Up @@ -95,7 +101,7 @@ impl Model {
data: VisualizationUpdateData,
) {
if let Some(view_id) = self.state.view_id_of_ast_node(target) {
match deserialize_visualization_data(data) {
match visualization_view::Data::json(&data) {
Ok(data) => update_endpoint.emit((view_id, data)),
Err(err) => {
// TODO [mwu]: We should consider having the visualization also accept error
Expand Down Expand Up @@ -178,6 +184,7 @@ impl Visualization {
manager: manager.clone_ref(),
error_manager: error_manager.clone_ref(),
state,
shown: default(),
});

frp::extend! { network
Expand Down Expand Up @@ -296,18 +303,3 @@ impl Visualization {
self
}
}



// ========================
// === Helper Functions ===
// ========================

fn deserialize_visualization_data(
data: VisualizationUpdateData,
) -> FallibleResult<visualization_view::Data> {
let binary = data.as_ref();
let as_text = std::str::from_utf8(binary)?;
let as_json: serde_json::Value = serde_json::from_str(as_text)?;
Ok(visualization_view::Data::from(as_json))
}
4 changes: 2 additions & 2 deletions app/gui/view/examples/visualization/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ fn init(app: &Application) {
let data = generate_data(
(time_info.since_animation_loop_started.unchecked_raw() / 1000.0).into(),
);
let content = serde_json::to_value(data).unwrap();
let data = Data::from(content);
let content = serde_json::to_string(&data).unwrap();
let data = Data::json(content.as_bytes()).unwrap();

visualization.send_data.emit(data);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,10 @@ pub struct BubbleChartModel {
}

impl BubbleChartModel {
#[profile(Debug)]
#[allow(clippy::question_mark)]
fn receive_data(&self, data: &Data) -> Result<(), DataError> {
let data_inner = match data {
Data::Json { content } => content,
_ => return Err(DataError::BinaryNotSupported),
};
let data_inner: &serde_json::Value = data_inner;
let data_inner: Rc<Vec<Vector3<f32>>> =
if let Ok(result) = serde_json::from_value(data_inner.clone()) {
result
} else {
return Err(DataError::InvalidJsonText);
};
let data_inner: Rc<Vec<Vector3<f32>>> = data.as_json()?.deserialize()?;

// Avoid re-creating views, if we have already created some before.
let mut views = self.views.borrow_mut();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl Error {

/// Sets the visualization data directly from the [`Input`] structure (not from the serialized
/// JSON).
pub fn set_data(&self, input: &Input) {
pub fn set_data(&self, input: Input) {
self.model.set_data(input);
}

Expand Down Expand Up @@ -207,24 +207,19 @@ impl Model {
self.reload_style();
}

#[profile(Debug)]
fn receive_data(&self, data: &Data) -> Result<(), DataError> {
match data {
Data::Json { content } => {
let input_result = serde_json::from_value(content.deref().clone());
let input: Input = input_result.map_err(|_| DataError::InvalidDataType)?;
self.set_data(&input);
Ok(())
}
Data::Binary => Err(DataError::BinaryNotSupported),
}
let input: Input = data.as_json()?.deserialize()?;
self.set_data(input);
Ok(())
}

fn set_data(&self, input: &Input) {
fn set_data(&self, input: Input) {
if let Some(kind) = input.kind {
self.messages.insert(kind, input.message.clone().into());
if kind == self.displayed.get() {
self.dom.dom().set_inner_text(&input.message);
}
self.messages.insert(kind, input.message.into());
}
// else we don't update the text, as the node does not contain error anymore. The
// visualization will be hidden once we receive expression update message.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,30 +269,19 @@ struct LazyGridDataUpdate {
impl TryFrom<visualization::Data> for LazyGridDataUpdate {
type Error = visualization::DataError;

#[profile(Debug)]
fn try_from(data: visualization::Data) -> Result<Self, Self::Error> {
if let visualization::Data::Json { content } = data {
let grid_data = serde_json::from_value(content.deref().clone());
let (data, update_type) = match grid_data {
Ok(data) => (data, UpdateType::PartialUpdate),
Err(_) => {
let data_str = if content.is_string() {
// We need to access the content `as_str` to preserve newlines. Just using
// `content.to_string()` would turn them into the characters `\n` in the
// output. The unwrap can never fail, as we just
// checked that the content is a string.
Ok(content.as_str().map(|s| s.to_owned()).unwrap_or_default())
} else {
serde_json::to_string_pretty(&*content)
};
let data_str =
data_str.unwrap_or_else(|e| format!("<Cannot render data: {e}.>"));
(data_str.into(), UpdateType::FullUpdate)
}
};
Ok(LazyGridDataUpdate { data, update_type })
} else {
Err(visualization::DataError::BinaryNotSupported)
}
let content = data.as_json()?;
let grid_data: Result<LazyGridData, _> = content.deserialize();
let (data, update_type) = match grid_data {
Ok(data) => (data, UpdateType::PartialUpdate),
Err(_) => {
let data_str = content.to_string();
let data_str = data_str.unwrap_or_else(|e| format!("<Cannot render data: {e}.>"));
(data_str.into(), UpdateType::FullUpdate)
}
};
Ok(LazyGridDataUpdate { data, update_type })
}
}

Expand Down
2 changes: 1 addition & 1 deletion app/gui/view/graph-editor/src/component/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ impl NodeModel {
if let Some(error) = error {
self.error_visualization.display_kind(*error.kind);
if let Some(error_data) = error.visualization_data() {
self.error_visualization.set_data(&error_data);
self.error_visualization.set_data(error_data);
}
if error.should_display() {
self.display_object.add_child(&self.error_visualization);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ impl ContainerModel {
preprocessor.emit(visualization.on_preprocessor_change.value());
}

#[profile(Debug)]
fn set_visualization_data(&self, data: &visualization::Data) {
self.visualization.borrow().for_each_ref(|vis| vis.send_data.emit(data))
}
Expand Down Expand Up @@ -498,6 +499,7 @@ impl ContainerModel {
self.view.overlay.is_this_target(target)
}

#[profile(Debug)]
fn next_visualization(
&self,
current_vis: &Option<visualization::Definition>,
Expand Down
57 changes: 43 additions & 14 deletions app/gui/view/graph-editor/src/component/visualization/data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,43 @@ use crate::prelude::*;

/// Json representation with a fast clone operation. Used for transmitting visualization data via
/// FRP networks.
#[derive(Clone, CloneRef, Debug, Default)]
#[derive(Clone, CloneRef, Debug)]
pub struct Json {
rc: Rc<serde_json::Value>,
rc: Rc<serde_json::value::RawValue>,
}

impl Deref for Json {
type Target = serde_json::Value;
fn deref(&self) -> &Self::Target {
&self.rc
impl Default for Json {
fn default() -> Self {
let null = serde_json::Value::default();
let raw_null = serde_json::value::to_raw_value(&null).unwrap();
let rc = raw_null.into();
Self { rc }
}
}

impl From<serde_json::Value> for Json {
fn from(t: serde_json::Value) -> Self {
let rc = Rc::new(t);
Self { rc }
impl Json {
/// Deserialize the JSON data into an arbitrary type.
#[profile(Debug)]
pub fn deserialize<T: serde::de::DeserializeOwned>(&self) -> Result<T, DataError> {
serde_json::from_str(self.rc.get()).map_err(|_| DataError::InvalidJsonText)
}

/// Produce a string from the value. If it's a JSON string, return it; otherwise, pretty-print
/// the JSON data.
#[profile(Debug)]
pub fn to_string(&self) -> serde_json::Result<String> {
let data_str: Result<String, _> = serde_json::from_str(self.rc.get());
data_str.or_else(|_| serde_json::to_string_pretty(&self.rc))
}

/// Return the raw data.
pub fn raw(&self) -> &str {
self.rc.get()
}
}



// ===================
// === Data Format ===
// ====================
Expand Down Expand Up @@ -87,10 +105,21 @@ impl Default for Data {
}
}

impl From<serde_json::Value> for Data {
fn from(t: serde_json::Value) -> Self {
let content = t.into();
Self::Json { content }
impl Data {
/// Deserialize JSON visualization data from the given bytes.
#[profile(Debug)]
pub fn json(data: &[u8]) -> FallibleResult<Self> {
let rc: Rc<serde_json::value::RawValue> = serde_json::from_slice(data)?;
let content = Json { rc };
Ok(Self::Json { content })
}

/// Get a reference to the contained JSON data. Fails if this is non-JSON (binary) data.
pub fn as_json(&self) -> Result<&Json, DataError> {
match self {
Data::Json { content } => Ok(content),
Data::Binary => Err(DataError::BinaryNotSupported),
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,17 +201,11 @@ impl InstanceModel {
#[cfg(not(target_arch = "wasm32"))]
fn set_size(&self, _size: Vector2) {}

#[profile(Debug)]
#[cfg(target_arch = "wasm32")]
fn receive_data(&self, data: &Data) -> result::Result<(), DataError> {
let data_json = match data {
Data::Json { content } => content,
_ => return Err(DataError::BinaryNotSupported),
};
let data_json: &serde_json::Value = data_json.deref();
let data_js = match json_to_value(data_json) {
Ok(value) => value,
Err(_) => return Err(DataError::InvalidDataType),
};
let data_json = data.as_json()?.raw();
let data_js = js_sys::JSON::parse(data_json).map_err(|_| DataError::InvalidDataType)?;
self.try_call1(&self.on_data_received, &data_js)
.map_err(|_| DataError::InternalComputationError)?;
Ok(())
Expand All @@ -227,6 +221,7 @@ impl InstanceModel {
self.object.emitPreprocessorChange()
}

#[profile(Debug)]
#[cfg(target_arch = "wasm32")]
/// Helper method to call methods on the wrapped javascript object.
fn try_call1(
Expand Down Expand Up @@ -353,17 +348,3 @@ fn get_method(
) -> Result<web::Function> {
Ok(default())
}

/// Convert the given JSON value to a `JsValue`.
///
/// Note that we need to use special serializer, as `serde_wasm_bindgen` defaults to outputting
/// some special `Map` type that is not supported by the visualization API (rather than proper
/// objects).
#[cfg(target_arch = "wasm32")]
pub fn json_to_value(
json: &serde_json::Value,
) -> std::result::Result<JsValue, serde_wasm_bindgen::Error> {
use serde::Serialize;
let serializer = serde_wasm_bindgen::Serializer::json_compatible();
json.serialize(&serializer)
}
Loading

0 comments on commit 5de0e6d

Please sign in to comment.