From 10341e3776be26f0f7ad24e3d4ed30bee1ce2b03 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Sun, 5 Jun 2022 17:47:31 -0400 Subject: [PATCH 1/7] Major Performance Improvements (#548) * Use WeakDom::into_raw for faster snapshot generation from models * Make compute_patch_set take snapshots by value * Stop deferring property application in apply_patch_set * Use InstanceBuilder::empty to avoid extra name allocations * Git dependencies, skip dropping ServeSession * Use std::mem::forget instead of ManuallyDrop * Switch to latest rbx-dom crates.io dependencies * Update other dependencies --- Cargo.lock | 161 +++++++++++++++--------------- Cargo.toml | 2 +- src/change_processor.rs | 4 +- src/cli/build.rs | 5 + src/serve_session.rs | 2 +- src/snapshot/instance_snapshot.rs | 22 ++-- src/snapshot/patch_apply.rs | 86 ++++++++-------- src/snapshot/patch_compute.rs | 47 +++++---- src/snapshot/tests/compute.rs | 10 +- src/snapshot/tree.rs | 5 +- src/snapshot_middleware/rbxm.rs | 3 +- src/snapshot_middleware/rbxmx.rs | 3 +- 12 files changed, 182 insertions(+), 168 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5b2a42df8..45e9c897d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -49,9 +49,9 @@ checksum = "a4c527152e37cf757a3f78aae5a06fbeefdb07ccc535c980a3208ee3060dd544" [[package]] name = "arrayvec" -version = "0.5.2" +version = "0.7.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "23b62fc65de8e4e7f52534fb52b0f3ed04746ae267519eef2a83941e8085068b" +checksum = "8da52d66c7071e2e3fa2a1e5c6d088fec47b593032b254f5e980de8ea54454d6" [[package]] name = "atty" @@ -114,17 +114,16 @@ checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" [[package]] name = "blake3" -version = "0.1.5" +version = "1.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "46080006c1505f12f64dd2a09264b343381ed3190fa02c8005d5d662ac571c63" +checksum = "a08e53fc5a564bb15bfe6fae56bd71522205f1f91893f9c0116edad6496c183f" dependencies = [ "arrayref", "arrayvec", "cc", - "cfg-if 0.1.10", + "cfg-if 1.0.0", "constant_time_eq", - "crypto-mac", - "digest", + "digest 0.10.3", ] [[package]] @@ -136,7 +135,16 @@ dependencies = [ "block-padding", "byte-tools", "byteorder", - "generic-array", + "generic-array 0.12.4", +] + +[[package]] +name = "block-buffer" +version = "0.10.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0bf7fe51849ea569fd452f37822f606a5cabb684dc918707a0193fd4664ff324" +dependencies = [ + "generic-array 0.14.5", ] [[package]] @@ -162,9 +170,9 @@ dependencies = [ [[package]] name = "bumpalo" -version = "3.9.1" +version = "3.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4a45a46ab1f2412e53d3a0ade76ffad2025804294569aae387231a0cd6e0899" +checksum = "37ccbd214614c6783386c1af30caf03192f17891059cecc394b4fb119e363de3" [[package]] name = "byte-tools" @@ -378,13 +386,13 @@ dependencies = [ ] [[package]] -name = "crypto-mac" -version = "0.7.0" +name = "crypto-common" +version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4434400df11d95d556bac068ddfedd482915eb18fe8bea89bc80b6e4b1c179e5" +checksum = "57952ca27b5e3606ff4dd79b0020231aaf9d6aa76dc05fd30137538c50bd3ce8" dependencies = [ - "generic-array", - "subtle", + "generic-array 0.14.5", + "typenum", ] [[package]] @@ -431,7 +439,18 @@ version = "0.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f3d0c8c8752312f9713efd397ff63acb9f85585afbf179282e720e7704954dd5" dependencies = [ - "generic-array", + "generic-array 0.12.4", +] + +[[package]] +name = "digest" +version = "0.10.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2fb860ca6fafa5552fb6d0e816a69c8e49f0908bf524e30a90d97c85892d506" +dependencies = [ + "block-buffer 0.10.2", + "crypto-common", + "subtle", ] [[package]] @@ -713,14 +732,13 @@ dependencies = [ ] [[package]] -name = "getrandom" -version = "0.1.16" +name = "generic-array" +version = "0.14.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8fc3cb4d91f53b50155bdcfd23f6a4c39ae1969c2ae85982b135750cccaf5fce" +checksum = "fd48d33ec7f05fbfa152300fdad764757cbded343c1aa1cff2fbaf4134851803" dependencies = [ - "cfg-if 1.0.0", - "libc", - "wasi 0.9.0+wasi-snapshot-preview1", + "typenum", + "version_check", ] [[package]] @@ -731,7 +749,7 @@ checksum = "9be70c98951c83b8d2f8f60d7065fa6d5146873094452a1008da8c2f1e4205ad" dependencies = [ "cfg-if 1.0.0", "libc", - "wasi 0.10.0+wasi-snapshot-preview1", + "wasi 0.10.2+wasi-snapshot-preview1", ] [[package]] @@ -847,9 +865,9 @@ checksum = "9a3a5bfb195931eeb336b2a7b4d761daec841b97f947d34394601737a7bba5e4" [[package]] name = "hyper" -version = "0.14.18" +version = "0.14.19" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b26ae0a80afebe130861d90abf98e3814a4f28a4c6ffeb5ab8ebb2be311e0ef2" +checksum = "42dc3c131584288d375f2d07f822b0cb012d8c6fb899a5b9fdb3cb7eb9b6004f" dependencies = [ "bytes", "futures-channel", @@ -895,9 +913,9 @@ dependencies = [ [[package]] name = "indexmap" -version = "1.8.1" +version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0f647032dfaa1f8b6dc29bd3edb7bbef4861b8b8007ebb118d6db284fd59f6ee" +checksum = "e6012d540c5baa3589337a98ce73408de9b5a25ec9fc2c6fd6be8f0d39e0ca5a" dependencies = [ "autocfg", "hashbrown", @@ -925,9 +943,9 @@ dependencies = [ [[package]] name = "insta" -version = "1.14.0" +version = "1.14.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "689960f187c43c01650c805fb6bc6f55ab944499d86d4ffe9474ad78991d8e94" +checksum = "bcc3e639bcba360d9237acabd22014c16f3df772db463b7446cd81b070714767" dependencies = [ "console", "once_cell", @@ -1129,9 +1147,9 @@ checksum = "2a60c7ce501c71e03a9c9c0d35b861413ae925bd979cc7a4e30d060069aaac8d" [[package]] name = "miniz_oxide" -version = "0.5.1" +version = "0.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d2b29bd4bc3f33391105ebee3589c19197c4271e3e5a9ec9bfe8127eeff8f082" +checksum = "6f5c75688da582b8ffc1f1799e9db273f32133c49e048f614d22ec3256773ccc" dependencies = [ "adler", ] @@ -1268,9 +1286,9 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.11.0" +version = "1.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7b10983b38c53aebdf33f542c6275b0f58a238129d00c4ae0e6fb59738d783ca" +checksum = "7709cef83f0c1f58f666e746a08b21e0085f7440fa6a29cc194d68aac97a4225" [[package]] name = "oorandom" @@ -1328,9 +1346,9 @@ checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf" [[package]] name = "openssl-sys" -version = "0.9.73" +version = "0.9.74" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9d5fd19fb3e0a8191c1e34935718976a3e70c112ab9a24af6d7cadccd9d90bc0" +checksum = "835363342df5fba8354c5b453325b110ffd54044e588c539cf2f20a8014e4cb1" dependencies = [ "autocfg", "cc", @@ -1567,22 +1585,20 @@ dependencies = [ [[package]] name = "rand" -version = "0.7.3" +version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a6b1679d49b24bbfe0c803429aa1874472f50d9b363131f0e89fc356b544d03" +checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404" dependencies = [ - "getrandom 0.1.16", "libc", "rand_chacha", "rand_core", - "rand_hc", ] [[package]] name = "rand_chacha" -version = "0.2.2" +version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f4c8ed856279c9737206bf725bf36935d8666ead7aa69b52be55af369d193402" +checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" dependencies = [ "ppv-lite86", "rand_core", @@ -1590,20 +1606,11 @@ dependencies = [ [[package]] name = "rand_core" -version = "0.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "90bde5296fc891b0cef12a6d03ddccc162ce7b2aff54160af9338f8d40df6d19" -dependencies = [ - "getrandom 0.1.16", -] - -[[package]] -name = "rand_hc" -version = "0.2.0" +version = "0.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ca3129af7b92a17112d59ad498c6f81eaf463253766b90396d39ea7a39d6613c" +checksum = "d34f1408f55294453790c48b2f1ebbb1c5b4b7563eb1f418bcfcfdbb06ebb4e7" dependencies = [ - "rand_core", + "getrandom", ] [[package]] @@ -1646,9 +1653,9 @@ dependencies = [ [[package]] name = "rbx_dom_weak" -version = "2.3.0" +version = "2.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6f7f524fb18f30d7065c82c4e87f747705679329810207e96169c6d4ec922d1f" +checksum = "b7cc2238fd858d706f4f5c6a0f9af3cd4a10f3f8dc8b6534e683f8543af26f33" dependencies = [ "rbx_types", "serde", @@ -1678,11 +1685,11 @@ dependencies = [ [[package]] name = "rbx_types" -version = "1.3.0" +version = "1.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d637383aa560cb675b7ea7a7778b945dab065ccc7c158f77b5455e27efadc6df" +checksum = "cbfc0ca9c674968170d4fbbd95dc692d0b3f9405b4830babc76107dc00a66380" dependencies = [ - "base64 0.11.0", + "base64 0.13.0", "bitflags", "blake3", "lazy_static", @@ -1720,7 +1727,7 @@ version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b033d837a7cf162d7993aded9304e30a83213c648b6e389db233191f891e5c2b" dependencies = [ - "getrandom 0.2.6", + "getrandom", "redox_syscall", "thiserror", ] @@ -2069,8 +2076,8 @@ version = "0.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f7d94d0bede923b3cea61f3f1ff57ff8cdfd77b400fb8f9998949e0cf04163df" dependencies = [ - "block-buffer", - "digest", + "block-buffer 0.7.3", + "digest 0.8.1", "fake-simd", "opaque-debug", ] @@ -2129,15 +2136,15 @@ checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" [[package]] name = "subtle" -version = "1.0.0" +version = "2.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d67a5a62ba6e01cb2192ff309324cb4875d0c451d55fe2319433abe7a05a8ee" +checksum = "6bdef32e8150c2a081110b42772ffe7d7c9032b606bc226c8260fd97e0976601" [[package]] name = "syn" -version = "1.0.95" +version = "1.0.96" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fbaf6116ab8924f39d52792136fb74fd60a80194cf1b1c6ffa6453eef1c3f942" +checksum = "0748dd251e24453cb8717f0354206b91557e4ec8703673a4b30208f2abaf1ebf" dependencies = [ "proc-macro2 1.0.39", "quote 1.0.18", @@ -2248,9 +2255,9 @@ checksum = "cda74da7e1a664f795bb1f8a87ec406fb89a02522cf6e50620d016add6dbbf5c" [[package]] name = "tokio" -version = "1.18.2" +version = "1.19.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4903bf0427cf68dddd5aa6a93220756f8be0c34fcfa9f5e6191e103e15a31395" +checksum = "95eec79ea28c00a365f539f1961e9278fbcaf81c0ff6aaf0e93c181352446948" dependencies = [ "bytes", "libc", @@ -2275,9 +2282,9 @@ dependencies = [ [[package]] name = "tokio-util" -version = "0.7.2" +version = "0.7.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f988a1a1adc2fb21f9c12aa96441da33a1728193ae0b95d2be22dbd17fcb4e5c" +checksum = "cc463cd8deddc3770d20f9852143d50bf6094e640b485cb2e189a2099085ff45" dependencies = [ "bytes", "futures-core", @@ -2449,11 +2456,11 @@ dependencies = [ [[package]] name = "uuid" -version = "1.0.0" +version = "1.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8cfcd319456c4d6ea10087ed423473267e1a071f3bc0aa89f80d60997843c6f0" +checksum = "c6d5d669b51467dcf7b2f1a796ce0f955f05f01cafda6c19d6e95f730df29238" dependencies = [ - "getrandom 0.2.6", + "getrandom", "serde", ] @@ -2518,15 +2525,9 @@ dependencies = [ [[package]] name = "wasi" -version = "0.9.0+wasi-snapshot-preview1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cccddf32554fecc6acb585f82a32a72e28b48f8c4c1883ddfeeeaa96f7d8e519" - -[[package]] -name = "wasi" -version = "0.10.0+wasi-snapshot-preview1" +version = "0.10.2+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a143597ca7c7793eff794def352d41792a93c481eb1042423ff7ff72ba2c31f" +checksum = "fd6fbd9a79829dd1ad0cc20627bf1ed606756a7f77edff7b66b7064f9cb327c6" [[package]] name = "wasi" diff --git a/Cargo.toml b/Cargo.toml index 9e5c3debd..83749b327 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,7 +51,7 @@ memofs = { version = "0.2.0", path = "crates/memofs" } # rbx_xml = { path = "../rbx-dom/rbx_xml" } rbx_binary = "0.6.4" -rbx_dom_weak = "2.3.0" +rbx_dom_weak = "2.4.0" rbx_reflection = "4.2.0" rbx_reflection_database = "0.2.2" rbx_xml = "0.12.3" diff --git a/src/change_processor.rs b/src/change_processor.rs index 7082a687b..dce1cd8d7 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -291,7 +291,7 @@ fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option< } }; - let patch_set = compute_patch_set(snapshot.as_ref(), &tree, id); + let patch_set = compute_patch_set(snapshot, &tree, id); apply_patch_set(tree, patch_set) } Ok(None) => { @@ -334,7 +334,7 @@ fn compute_and_apply_changes(tree: &mut RojoTree, vfs: &Vfs, id: Ref) -> Option< } }; - let patch_set = compute_patch_set(snapshot.as_ref(), &tree, id); + let patch_set = compute_patch_set(snapshot, &tree, id); apply_patch_set(tree, patch_set) } }; diff --git a/src/cli/build.rs b/src/cli/build.rs index 24f835469..d6c3f3499 100644 --- a/src/cli/build.rs +++ b/src/cli/build.rs @@ -1,5 +1,6 @@ use std::{ io::{BufWriter, Write}, + mem::forget, path::{Path, PathBuf}, }; @@ -61,6 +62,10 @@ impl BuildCommand { } } + // Avoid dropping ServeSession: it's potentially VERY expensive to drop + // and we're about to exit anyways. + forget(session); + Ok(()) } } diff --git a/src/serve_session.rs b/src/serve_session.rs index a4eced447..96a68ac9d 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -130,7 +130,7 @@ impl ServeSession { let snapshot = snapshot_from_vfs(&instance_context, &vfs, &start_path)?; log::trace!("Computing initial patch set"); - let patch_set = compute_patch_set(snapshot.as_ref(), &tree, root_id); + let patch_set = compute_patch_set(snapshot, &tree, root_id); log::trace!("Applying initial patch set"); apply_patch_set(&mut tree, patch_set); diff --git a/src/snapshot/instance_snapshot.rs b/src/snapshot/instance_snapshot.rs index 77e9f3c67..36c518212 100644 --- a/src/snapshot/instance_snapshot.rs +++ b/src/snapshot/instance_snapshot.rs @@ -4,7 +4,7 @@ use std::{borrow::Cow, collections::HashMap}; use rbx_dom_weak::{ types::{Ref, Variant}, - WeakDom, + Instance, WeakDom, }; use serde::{Deserialize, Serialize}; @@ -103,22 +103,28 @@ impl InstanceSnapshot { } #[profiling::function] - pub fn from_tree(tree: &WeakDom, id: Ref) -> Self { - let instance = tree.get_by_ref(id).expect("instance did not exist in tree"); + pub fn from_tree(tree: WeakDom, id: Ref) -> Self { + let (_, mut raw_tree) = tree.into_raw(); + Self::from_raw_tree(&mut raw_tree, id) + } + + fn from_raw_tree(raw_tree: &mut HashMap, id: Ref) -> Self { + let instance = raw_tree + .remove(&id) + .expect("instance did not exist in tree"); let children = instance .children() .iter() - .copied() - .map(|id| Self::from_tree(tree, id)) + .map(|&id| Self::from_raw_tree(raw_tree, id)) .collect(); Self { snapshot_id: Some(id), metadata: InstanceMetadata::default(), - name: Cow::Owned(instance.name.clone()), - class_name: Cow::Owned(instance.class.clone()), - properties: instance.properties.clone(), + name: Cow::Owned(instance.name), + class_name: Cow::Owned(instance.class), + properties: instance.properties, children, } } diff --git a/src/snapshot/patch_apply.rs b/src/snapshot/patch_apply.rs index f87660ba6..439452d82 100644 --- a/src/snapshot/patch_apply.rs +++ b/src/snapshot/patch_apply.rs @@ -1,6 +1,9 @@ //! Defines the algorithm for applying generated patches. -use std::collections::HashMap; +use std::{ + collections::{HashMap, HashSet}, + mem::take, +}; use rbx_dom_weak::types::{Ref, Variant}; @@ -16,18 +19,27 @@ use super::{ pub fn apply_patch_set(tree: &mut RojoTree, patch_set: PatchSet) -> AppliedPatchSet { let mut context = PatchApplyContext::default(); - for removed_id in patch_set.removed_instances { - apply_remove_instance(&mut context, tree, removed_id); + { + profiling::scope!("removals"); + for removed_id in patch_set.removed_instances { + apply_remove_instance(&mut context, tree, removed_id); + } } - for add_patch in patch_set.added_instances { - apply_add_child(&mut context, tree, add_patch.parent_id, add_patch.instance); + { + profiling::scope!("additions"); + for add_patch in patch_set.added_instances { + apply_add_child(&mut context, tree, add_patch.parent_id, add_patch.instance); + } } - // Updates need to be applied after additions, which reduces the complexity - // of updates significantly. - for update_patch in patch_set.updated_instances { - apply_update_child(&mut context, tree, update_patch); + { + profiling::scope!("updates"); + // Updates need to be applied after additions, which reduces the complexity + // of updates significantly. + for update_patch in patch_set.updated_instances { + apply_update_child(&mut context, tree, update_patch); + } } finalize_patch_application(context, tree) @@ -56,20 +68,9 @@ struct PatchApplyContext { /// eachother. snapshot_id_to_instance_id: HashMap, - /// The properties of instances added by the current `PatchSet`. - /// - /// Instances added to the tree can refer to eachother via Ref properties, - /// but we need to make sure they're correctly transformed from snapshot - /// space into tree space (via `snapshot_id_to_instance_id`). - /// - /// It's not possible to do that transformation for refs that refer to added - /// instances until all the instances have actually been inserted into the - /// tree. For simplicity, we defer application of _all_ properties on added - /// instances instead of just Refs. - /// - /// This doesn't affect updated instances, since they're always applied - /// after we've added all the instances from the patch. - added_instance_properties: HashMap>, + /// Tracks all of the instances added by this patch that have refs that need + /// to be rewritten. + has_refs_to_rewrite: HashSet, /// The current applied patch result, describing changes made to the tree. applied_patch_set: AppliedPatchSet, @@ -85,23 +86,22 @@ struct PatchApplyContext { /// The remaining Ref properties need to be handled during patch application, /// where we build up a map of snapshot IDs to instance IDs as they're created, /// then apply properties all at once at the end. +#[profiling::function] fn finalize_patch_application(context: PatchApplyContext, tree: &mut RojoTree) -> AppliedPatchSet { - for (id, properties) in context.added_instance_properties { + for id in context.has_refs_to_rewrite { // This should always succeed since instances marked as added in our // patch should be added without fail. let mut instance = tree .get_instance_mut(id) .expect("Invalid instance ID in deferred property map"); - for (key, mut property_value) in properties { - if let Variant::Ref(referent) = property_value { + for value in instance.properties_mut().values_mut() { + if let Variant::Ref(referent) = value { if let Some(&instance_referent) = context.snapshot_id_to_instance_id.get(&referent) { - property_value = Variant::Ref(instance_referent); + *value = Variant::Ref(instance_referent); } } - - instance.properties_mut().insert(key, property_value); } } @@ -117,24 +117,24 @@ fn apply_add_child( context: &mut PatchApplyContext, tree: &mut RojoTree, parent_id: Ref, - snapshot: InstanceSnapshot, + mut snapshot: InstanceSnapshot, ) { let snapshot_id = snapshot.snapshot_id; - let properties = snapshot.properties; - let children = snapshot.children; - - // Property application is deferred until after all children - // are constructed. This helps apply referents correctly. - let remaining_snapshot = InstanceSnapshot::new() - .name(snapshot.name) - .class_name(snapshot.class_name) - .metadata(snapshot.metadata) - .snapshot_id(snapshot.snapshot_id); - - let id = tree.insert_instance(parent_id, remaining_snapshot); + let children = take(&mut snapshot.children); + + // If an object we're adding has a non-null referent, we'll note this + // instance down as needing to be revisited later. + let has_refs = snapshot.properties.values().any(|value| match value { + Variant::Ref(value) => value.is_some(), + _ => false, + }); + + let id = tree.insert_instance(parent_id, snapshot); context.applied_patch_set.added.push(id); - context.added_instance_properties.insert(id, properties); + if has_refs { + context.has_refs_to_rewrite.insert(id); + } if let Some(snapshot_id) = snapshot_id { context.snapshot_id_to_instance_id.insert(snapshot_id, id); diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index c778f1fab..23e2f3024 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -1,7 +1,10 @@ //! Defines the algorithm for computing a roughly-minimal patch set given an //! existing instance tree and an instance snapshot. -use std::collections::{HashMap, HashSet}; +use std::{ + collections::{HashMap, HashSet}, + mem::take, +}; use rbx_dom_weak::types::{Ref, Variant}; @@ -11,11 +14,7 @@ use super::{ }; #[profiling::function] -pub fn compute_patch_set( - snapshot: Option<&InstanceSnapshot>, - tree: &RojoTree, - id: Ref, -) -> PatchSet { +pub fn compute_patch_set(snapshot: Option, tree: &RojoTree, id: Ref) -> PatchSet { let mut patch_set = PatchSet::new(); if let Some(snapshot) = snapshot { @@ -75,7 +74,7 @@ fn rewrite_refs_in_snapshot(context: &ComputePatchContext, snapshot: &mut Instan fn compute_patch_set_internal( context: &mut ComputePatchContext, - snapshot: &InstanceSnapshot, + mut snapshot: InstanceSnapshot, tree: &RojoTree, id: Ref, patch_set: &mut PatchSet, @@ -88,12 +87,12 @@ fn compute_patch_set_internal( .get_instance(id) .expect("Instance did not exist in tree"); - compute_property_patches(snapshot, &instance, patch_set); - compute_children_patches(context, snapshot, tree, id, patch_set); + compute_property_patches(&mut snapshot, &instance, patch_set); + compute_children_patches(context, &mut snapshot, tree, id, patch_set); } fn compute_property_patches( - snapshot: &InstanceSnapshot, + snapshot: &mut InstanceSnapshot, instance: &InstanceWithMeta, patch_set: &mut PatchSet, ) { @@ -103,32 +102,32 @@ fn compute_property_patches( let changed_name = if snapshot.name == instance.name() { None } else { - Some(snapshot.name.clone().into_owned()) + Some(take(&mut snapshot.name).into_owned()) }; let changed_class_name = if snapshot.class_name == instance.class_name() { None } else { - Some(snapshot.class_name.clone().into_owned()) + Some(take(&mut snapshot.class_name).into_owned()) }; let changed_metadata = if &snapshot.metadata == instance.metadata() { None } else { - Some(snapshot.metadata.clone()) + Some(take(&mut snapshot.metadata)) }; - for (name, snapshot_value) in &snapshot.properties { - visited_properties.insert(name.as_str()); + for (name, snapshot_value) in take(&mut snapshot.properties) { + visited_properties.insert(name.clone()); - match instance.properties().get(name) { + match instance.properties().get(&name) { Some(instance_value) => { - if snapshot_value != instance_value { - changed_properties.insert(name.clone(), Some(snapshot_value.clone())); + if &snapshot_value != instance_value { + changed_properties.insert(name, Some(snapshot_value)); } } None => { - changed_properties.insert(name.clone(), Some(snapshot_value.clone())); + changed_properties.insert(name, Some(snapshot_value)); } } } @@ -160,7 +159,7 @@ fn compute_property_patches( fn compute_children_patches( context: &mut ComputePatchContext, - snapshot: &InstanceSnapshot, + snapshot: &mut InstanceSnapshot, tree: &RojoTree, id: Ref, patch_set: &mut PatchSet, @@ -173,7 +172,7 @@ fn compute_children_patches( let mut paired_instances = vec![false; instance_children.len()]; - for snapshot_child in snapshot.children.iter() { + for snapshot_child in take(&mut snapshot.children) { let matching_instance = instance_children .iter() @@ -210,7 +209,7 @@ fn compute_children_patches( None => { patch_set.added_instances.push(PatchAdd { parent_id: id, - instance: snapshot_child.clone(), + instance: snapshot_child, }); } } @@ -258,7 +257,7 @@ mod test { children: Vec::new(), }; - let patch_set = compute_patch_set(Some(&snapshot), &tree, root_id); + let patch_set = compute_patch_set(Some(snapshot), &tree, root_id); let expected_patch_set = PatchSet { updated_instances: vec![PatchUpdate { @@ -308,7 +307,7 @@ mod test { class_name: Cow::Borrowed("foo"), }; - let patch_set = compute_patch_set(Some(&snapshot), &tree, root_id); + let patch_set = compute_patch_set(Some(snapshot), &tree, root_id); let expected_patch_set = PatchSet { added_instances: vec![PatchAdd { diff --git a/src/snapshot/tests/compute.rs b/src/snapshot/tests/compute.rs index 7b15d3782..309d73b6e 100644 --- a/src/snapshot/tests/compute.rs +++ b/src/snapshot/tests/compute.rs @@ -23,7 +23,7 @@ fn set_name_and_class_name() { children: Vec::new(), }; - let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id()); + let patch_set = compute_patch_set(Some(snapshot), &tree, tree.get_root_id()); let patch_value = redactions.redacted_yaml(patch_set); assert_yaml_snapshot!(patch_value); @@ -47,7 +47,7 @@ fn set_property() { children: Vec::new(), }; - let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id()); + let patch_set = compute_patch_set(Some(snapshot), &tree, tree.get_root_id()); let patch_value = redactions.redacted_yaml(patch_set); assert_yaml_snapshot!(patch_value); @@ -78,7 +78,7 @@ fn remove_property() { children: Vec::new(), }; - let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id()); + let patch_set = compute_patch_set(Some(snapshot), &tree, tree.get_root_id()); let patch_value = redactions.redacted_yaml(patch_set); assert_yaml_snapshot!(patch_value); @@ -107,7 +107,7 @@ fn add_child() { }], }; - let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id()); + let patch_set = compute_patch_set(Some(snapshot), &tree, tree.get_root_id()); let patch_value = redactions.redacted_yaml(patch_set); assert_yaml_snapshot!(patch_value); @@ -139,7 +139,7 @@ fn remove_child() { children: Vec::new(), }; - let patch_set = compute_patch_set(Some(&snapshot), &tree, tree.get_root_id()); + let patch_set = compute_patch_set(Some(snapshot), &tree, tree.get_root_id()); let patch_value = redactions.redacted_yaml(patch_set); assert_yaml_snapshot!(patch_value); diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index b0f12b69a..e342e1862 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -87,8 +87,9 @@ impl RojoTree { } pub fn insert_instance(&mut self, parent_ref: Ref, snapshot: InstanceSnapshot) -> Ref { - let builder = InstanceBuilder::new(snapshot.class_name.to_owned()) - .with_name(snapshot.name.to_owned()) + let builder = InstanceBuilder::empty() + .with_class(snapshot.class_name.into_owned()) + .with_name(snapshot.name.into_owned()) .with_properties(snapshot.properties); let referent = self.inner.insert(parent_ref, builder); diff --git a/src/snapshot_middleware/rbxm.rs b/src/snapshot_middleware/rbxm.rs index c94d4ef4d..969f022dc 100644 --- a/src/snapshot_middleware/rbxm.rs +++ b/src/snapshot_middleware/rbxm.rs @@ -22,7 +22,8 @@ pub fn snapshot_rbxm( let children = root_instance.children(); if children.len() == 1 { - let snapshot = InstanceSnapshot::from_tree(&temp_tree, children[0]) + let child = children[0]; + let snapshot = InstanceSnapshot::from_tree(temp_tree, child) .name(name) .metadata( InstanceMetadata::new() diff --git a/src/snapshot_middleware/rbxmx.rs b/src/snapshot_middleware/rbxmx.rs index 3cdd52e45..4f128ff6e 100644 --- a/src/snapshot_middleware/rbxmx.rs +++ b/src/snapshot_middleware/rbxmx.rs @@ -24,7 +24,8 @@ pub fn snapshot_rbxmx( let children = root_instance.children(); if children.len() == 1 { - let snapshot = InstanceSnapshot::from_tree(&temp_tree, children[0]) + let child = children[0]; + let snapshot = InstanceSnapshot::from_tree(temp_tree, child) .name(name) .metadata( InstanceMetadata::new() From c06463b61d781e7b5179d9a38c7a6ed04585d5f2 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Sun, 5 Jun 2022 17:48:17 -0400 Subject: [PATCH 2/7] Update CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3894731df..23494c959 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased Changes * Switched from structopt to clap for command line argument parsing. +* Significantly improved performance of building and serving. ([#548]) + +[#548]: https://github.com/rojo-rbx/rojo/pull/548 ## [7.1.1] - May 26, 2022 * Fixed sourcemap command not stripping paths correctly ([#544]) From e5dbee107330e234444e8ba2d789649745a28581 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Thu, 9 Jun 2022 21:42:37 -0400 Subject: [PATCH 3/7] Defer application of `init.meta.json` until after init Lua files. (#549) Fixes #546. --- .../end_to_end__tests__build__issue_546.snap | 14 +++++++ rojo-test/build-tests/issue_546/README.md | 2 + .../issue_546/default.project.json | 6 +++ .../issue_546/hello/init.client.lua | 1 + .../issue_546/hello/init.meta.json | 5 +++ src/snapshot_middleware/dir.rs | 41 ++++++++++++++++--- src/snapshot_middleware/lua.rs | 12 +++++- tests/tests/build.rs | 3 +- 8 files changed, 75 insertions(+), 9 deletions(-) create mode 100644 rojo-test/build-test-snapshots/end_to_end__tests__build__issue_546.snap create mode 100644 rojo-test/build-tests/issue_546/README.md create mode 100644 rojo-test/build-tests/issue_546/default.project.json create mode 100644 rojo-test/build-tests/issue_546/hello/init.client.lua create mode 100644 rojo-test/build-tests/issue_546/hello/init.meta.json diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__issue_546.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__issue_546.snap new file mode 100644 index 000000000..3107344a5 --- /dev/null +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__issue_546.snap @@ -0,0 +1,14 @@ +--- +source: tests/tests/build.rs +assertion_line: 98 +expression: contents +--- + + + + issue_546 + true + print("Hello, world!") + + + diff --git a/rojo-test/build-tests/issue_546/README.md b/rojo-test/build-tests/issue_546/README.md new file mode 100644 index 000000000..7ccd95fa6 --- /dev/null +++ b/rojo-test/build-tests/issue_546/README.md @@ -0,0 +1,2 @@ +# Issue #546 (https://github.com/rojo-rbx/rojo/issues/546) +Regression from Rojo 6.2.0 to Rojo 7.0.0. Meta files named as init.meta.json should apply after init.client.lua and other init files. \ No newline at end of file diff --git a/rojo-test/build-tests/issue_546/default.project.json b/rojo-test/build-tests/issue_546/default.project.json new file mode 100644 index 000000000..65c91a526 --- /dev/null +++ b/rojo-test/build-tests/issue_546/default.project.json @@ -0,0 +1,6 @@ +{ + "name": "issue_546", + "tree": { + "$path": "hello" + } +} \ No newline at end of file diff --git a/rojo-test/build-tests/issue_546/hello/init.client.lua b/rojo-test/build-tests/issue_546/hello/init.client.lua new file mode 100644 index 000000000..1385fe3e1 --- /dev/null +++ b/rojo-test/build-tests/issue_546/hello/init.client.lua @@ -0,0 +1 @@ +print("Hello, world!") \ No newline at end of file diff --git a/rojo-test/build-tests/issue_546/hello/init.meta.json b/rojo-test/build-tests/issue_546/hello/init.meta.json new file mode 100644 index 000000000..54f469252 --- /dev/null +++ b/rojo-test/build-tests/issue_546/hello/init.meta.json @@ -0,0 +1,5 @@ +{ + "properties": { + "Disabled": true + } +} \ No newline at end of file diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index bad35c813..404b794de 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -10,6 +10,40 @@ pub fn snapshot_dir( context: &InstanceContext, vfs: &Vfs, path: &Path, +) -> anyhow::Result> { + let mut snapshot = match snapshot_dir_no_meta(context, vfs, path)? { + Some(snapshot) => snapshot, + None => return Ok(None), + }; + + if let Some(mut meta) = dir_meta(vfs, path)? { + meta.apply_all(&mut snapshot)?; + } + + Ok(Some(snapshot)) +} + +/// Retrieves the meta file that should be applied for this directory, if it +/// exists. +pub fn dir_meta(vfs: &Vfs, path: &Path) -> anyhow::Result> { + let meta_path = path.join("init.meta.json"); + + if let Some(meta_contents) = vfs.read(&meta_path).with_not_found()? { + let metadata = DirectoryMetadata::from_slice(&meta_contents, meta_path)?; + Ok(Some(metadata)) + } else { + Ok(None) + } +} + +/// Snapshot a directory without applying meta files; useful for if the +/// directory's ClassName will change before metadata should be applied. For +/// example, this can happen if the directory contains an `init.client.lua` +/// file. +pub fn snapshot_dir_no_meta( + context: &InstanceContext, + vfs: &Vfs, + path: &Path, ) -> anyhow::Result> { let passes_filter_rules = |child: &DirEntry| { context @@ -52,7 +86,7 @@ pub fn snapshot_dir( path.join("init.client.lua"), ]; - let mut snapshot = InstanceSnapshot::new() + let snapshot = InstanceSnapshot::new() .name(instance_name) .class_name("Folder") .children(snapshot_children) @@ -63,11 +97,6 @@ pub fn snapshot_dir( .context(context), ); - if let Some(meta_contents) = vfs.read(&meta_path).with_not_found()? { - let mut metadata = DirectoryMetadata::from_slice(&meta_contents, meta_path)?; - metadata.apply_all(&mut snapshot)?; - } - Ok(Some(snapshot)) } diff --git a/src/snapshot_middleware/lua.rs b/src/snapshot_middleware/lua.rs index 02bbc51d3..1b9117625 100644 --- a/src/snapshot_middleware/lua.rs +++ b/src/snapshot_middleware/lua.rs @@ -6,7 +6,11 @@ use memofs::{IoResultExt, Vfs}; use crate::snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot}; -use super::{dir::snapshot_dir, meta_file::AdjacentMetadata, util::match_trailing}; +use super::{ + dir::{dir_meta, snapshot_dir_no_meta}, + meta_file::AdjacentMetadata, + util::match_trailing, +}; /// Core routine for turning Lua files into snapshots. pub fn snapshot_lua( @@ -66,7 +70,7 @@ pub fn snapshot_lua_init( init_path: &Path, ) -> anyhow::Result> { let folder_path = init_path.parent().unwrap(); - let dir_snapshot = snapshot_dir(context, vfs, folder_path)?.unwrap(); + let dir_snapshot = snapshot_dir_no_meta(context, vfs, folder_path)?.unwrap(); if dir_snapshot.class_name != "Folder" { anyhow::bail!( @@ -86,6 +90,10 @@ pub fn snapshot_lua_init( init_snapshot.children = dir_snapshot.children; init_snapshot.metadata = dir_snapshot.metadata; + if let Some(mut meta) = dir_meta(vfs, folder_path)? { + meta.apply_all(&mut init_snapshot)?; + } + Ok(Some(init_snapshot)) } diff --git a/tests/tests/build.rs b/tests/tests/build.rs index fc3b55feb..fc917d045 100644 --- a/tests/tests/build.rs +++ b/tests/tests/build.rs @@ -36,11 +36,13 @@ gen_build_tests! { init_meta_class_name, init_meta_properties, init_with_children, + issue_546, json_as_lua, json_model_in_folder, json_model_legacy_name, module_in_folder, module_init, + optional, project_composed_default, project_composed_file, project_root_name, @@ -53,7 +55,6 @@ gen_build_tests! { txt, txt_in_folder, unresolved_values, - optional, weldconstraint, } From 2e7c4b6dff7efe8a5ba794cfca467ee7cdcc6bdf Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Fri, 10 Jun 2022 01:55:16 -0400 Subject: [PATCH 4/7] Update Changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23494c959..797d1cccd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,10 @@ ## Unreleased Changes * Switched from structopt to clap for command line argument parsing. * Significantly improved performance of building and serving. ([#548]) +* Fixed `init.meta.json` when used with `init.lua` and related files. ([#549]) [#548]: https://github.com/rojo-rbx/rojo/pull/548 +[#549]: https://github.com/rojo-rbx/rojo/pull/549 ## [7.1.1] - May 26, 2022 * Fixed sourcemap command not stripping paths correctly ([#544]) From 2624ea7d2a3a9e3d724aae5c33d5c663ac970b10 Mon Sep 17 00:00:00 2001 From: Micah <48431591+nezuo@users.noreply.github.com> Date: Sat, 11 Jun 2022 23:57:40 -0700 Subject: [PATCH 5/7] Remove .luacheckrc (#551) --- .luacheckrc | 58 ----------------------------------------------------- 1 file changed, 58 deletions(-) delete mode 100644 .luacheckrc diff --git a/.luacheckrc b/.luacheckrc deleted file mode 100644 index bb55667f7..000000000 --- a/.luacheckrc +++ /dev/null @@ -1,58 +0,0 @@ -stds.roblox = { - read_globals = { - game = { - other_fields = true, - }, - - -- Roblox globals - "script", - - -- Extra functions - "tick", "warn", "spawn", - "wait", "settings", "typeof", - - -- Types - "Vector2", "Vector3", - "Vector2int16", "Vector3int16", - "Color3", - "UDim", "UDim2", - "Rect", - "CFrame", - "Enum", - "Instance", - "DockWidgetPluginGuiInfo", - } -} - -stds.plugin = { - read_globals = { - "plugin", - } -} - -stds.testez = { - read_globals = { - "describe", - "it", "itFOCUS", "itSKIP", "itFIXME", - "FOCUS", "SKIP", "HACK_NO_XPCALL", - "expect", - } -} - -ignore = { - "212", -- unused arguments - "421", -- shadowing local variable - "422", -- shadowing argument - "431", -- shadowing upvalue - "432", -- shadowing upvalue argument -} - -std = "lua51+roblox" - -files["**/*.server.lua"] = { - std = "+plugin", -} - -files["**/*.spec.lua"] = { - std = "+testez", -} \ No newline at end of file From cf76982cfafb6919caf4bc96ca5db3f05d7297d9 Mon Sep 17 00:00:00 2001 From: Micah <48431591+nezuo@users.noreply.github.com> Date: Sun, 12 Jun 2022 00:41:49 -0700 Subject: [PATCH 6/7] Update selene (#550) * Update selene * Update foreman.toml Co-authored-by: Sasial <44125644+sasial-dev@users.noreply.github.com> Co-authored-by: Sasial <44125644+sasial-dev@users.noreply.github.com> Co-authored-by: Lucien Greathouse --- .gitignore | 3 --- foreman.toml | 2 +- testez.toml | 66 ---------------------------------------------------- testez.yml | 53 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 70 deletions(-) delete mode 100644 testez.toml create mode 100644 testez.yml diff --git a/.gitignore b/.gitignore index b12513e17..88cdf551a 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,3 @@ # Snapshot files from the 'insta' Rust crate **/*.snap.new - -# Selene generates a roblox.toml file that should not be checked in. -/roblox.toml \ No newline at end of file diff --git a/foreman.toml b/foreman.toml index c1275b8db..20ce14cb6 100644 --- a/foreman.toml +++ b/foreman.toml @@ -1,4 +1,4 @@ [tools] rojo = { source = "rojo-rbx/rojo", version = "7.1.1" } run-in-roblox = { source = "rojo-rbx/run-in-roblox", version = "0.3.0" } -selene = { source = "Kampfkarren/selene", version = "0.17.0" } +selene = { source = "Kampfkarren/selene", version = "0.18.2" } diff --git a/testez.toml b/testez.toml deleted file mode 100644 index c2c5a984f..000000000 --- a/testez.toml +++ /dev/null @@ -1,66 +0,0 @@ -[[afterAll.args]] -type = "function" - -[[afterEach.args]] -type = "function" - -[[beforeAll.args]] -type = "function" - -[[beforeEach.args]] -type = "function" - -[[describe.args]] -type = "string" - -[[describe.args]] -type = "function" - -[[describeFOCUS.args]] -type = "string" - -[[describeFOCUS.args]] -type = "function" - -[[describeSKIP.args]] -type = "string" - -[[describeSKIP.args]] -type = "function" - -[[expect.args]] -type = "any" - -[[FIXME.args]] -type = "string" -required = false - -[FOCUS] -args = [] - -[[it.args]] -type = "string" - -[[it.args]] -type = "function" - -[[itFIXME.args]] -type = "string" - -[[itFIXME.args]] -type = "function" - -[[itFOCUS.args]] -type = "string" - -[[itFOCUS.args]] -type = "function" - -[[itSKIP.args]] -type = "string" - -[[itSKIP.args]] -type = "function" - -[SKIP] -args = [] diff --git a/testez.yml b/testez.yml new file mode 100644 index 000000000..08be74e19 --- /dev/null +++ b/testez.yml @@ -0,0 +1,53 @@ +--- +globals: + FIXME: + args: + - required: false + type: string + FOCUS: + args: [] + SKIP: + args: [] + afterAll: + args: + - type: function + afterEach: + args: + - type: function + beforeAll: + args: + - type: function + beforeEach: + args: + - type: function + describe: + args: + - type: string + - type: function + describeFOCUS: + args: + - type: string + - type: function + describeSKIP: + args: + - type: string + - type: function + expect: + args: + - type: any + it: + args: + - type: string + - type: function + itFIXME: + args: + - type: string + - type: function + itFOCUS: + args: + - type: string + - type: function + itSKIP: + args: + - type: string + - type: function From cd5d6fd15cc6225ced392528cbf67395298aa47b Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Sun, 12 Jun 2022 04:32:26 -0400 Subject: [PATCH 7/7] Add test project for tags --- test-projects/tags/default.project.json | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 test-projects/tags/default.project.json diff --git a/test-projects/tags/default.project.json b/test-projects/tags/default.project.json new file mode 100644 index 000000000..51abe57ce --- /dev/null +++ b/test-projects/tags/default.project.json @@ -0,0 +1,14 @@ +{ + "name": "tags", + "tree": { + "$className": "DataModel", + "Workspace": { + "Folder": { + "$className": "Folder", + "$properties": { + "Tags": ["Hello", "World"] + } + } + } + } +} \ No newline at end of file