From d535a2862d9685989dd295f6a5ff97ef3e59d23b Mon Sep 17 00:00:00 2001 From: Paul Tagliamonte Date: Thu, 19 Dec 2024 16:18:35 -0500 Subject: [PATCH] Implement some basic cache program generation (#4840) Implement some basic cache program generation A bit ago, @jessfraz added the ability to control reexecution from the executor. When we did this, it used the digest to determine if there was a code change rather than a non-code change (whitespace, comment, etc). This allows the creation of a new program to be run without clearing the scene. This, in conjunction with being able to delete Engine objects by ID will allow us to do some clever stuff when incrementally executing a program. I'm still working on something a bit more advanced, but a good first step to derisk some of the caching behavior here fully is to implement a basic "changed program" stub. This process the ast programs (old and new) if it doesn't exactly match. This would have been a complete refresh before this commit. 1) Check all overlapping top-level statements of the body of the new and old AST and ensure they all match. - If this is true, this means that one of the two AST programs has more elements then the other, and they all otherwise match (addition or deletion of the end of the program). We continue to #2 in this case. - If this is false, we have a meaingful difference in a section of overlapping code. This will result in a cache miss and rebuild the scene. We short-cut here and the scene is rebuilt. 2) Check the lengths of the two bodies. - If they're the same, we shouldn't have even been called. We will short-cut with a noop cache return (no clear, no program). - if the old ast is longer, we've removed instructions from the program. We can't delete things now, so this will result in a cache miss and rebuild the scene. We short-cut here and the scene is rebuilt. - If the new ast is longer, we have an insertion of code at the end. 3) construct a new program using only the new elements from the new ast, and return a `CacheResult` that *does not clear the scene*. This means nothing will be rebuilt, and only a new object will polp onto the scene. This is the first case where we diverge with existing behavior. Signed-off-by: Paul R. Tagliamonte --- e2e/playwright/editor-tests.spec.ts | 55 +++++++++++++++- src/wasm-lib/kcl/src/execution/mod.rs | 90 ++++++++++++++++++++++++--- 2 files changed, 137 insertions(+), 8 deletions(-) diff --git a/e2e/playwright/editor-tests.spec.ts b/e2e/playwright/editor-tests.spec.ts index a4251e9e2..7bfb41636 100644 --- a/e2e/playwright/editor-tests.spec.ts +++ b/e2e/playwright/editor-tests.spec.ts @@ -76,7 +76,7 @@ test.describe('Editor tests', () => { await u.openDebugPanel() await expect( page.locator('[data-receive-command-type="scene_clear_all"]') - ).toHaveCount(2) + ).toHaveCount(1) await expect( page.locator('[data-message-type="execution-done"]') ).toHaveCount(2) @@ -100,7 +100,60 @@ test.describe('Editor tests', () => { ).toHaveCount(3) await expect( page.locator('[data-receive-command-type="scene_clear_all"]') + ).toHaveCount(1) + }) + + test('ensure we use the cache, and do not clear on append', async ({ + homePage, + page, + }) => { + const u = await getUtils(page) + await page.setBodyDimensions({ width: 1000, height: 500 }) + + await homePage.goToModelingScene() + await u.waitForPageLoad() + + await u.codeLocator.click() + await page.keyboard.type(`sketch001 = startSketchOn('XY') + |> startProfileAt([-10, -10], %) + |> line([20, 0], %) + |> line([0, 20], %) + |> line([-20, 0], %) + |> close(%)`) + + // Ensure we execute the first time. + await u.openDebugPanel() + await expect( + page.locator('[data-receive-command-type="scene_clear_all"]') + ).toHaveCount(1) + await expect( + page.locator('[data-message-type="execution-done"]') ).toHaveCount(2) + + // Add whitespace to the end of the code. + await u.codeLocator.click() + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('ArrowDown') + await page.keyboard.press('End') + await page.keyboard.press('Enter') + await page.keyboard.press('Enter') + await page.keyboard.type('const x = 1') + await page.keyboard.press('Enter') + + await u.openDebugPanel() + await expect( + page.locator('[data-message-type="execution-done"]') + ).toHaveCount(3) + await expect( + page.locator('[data-receive-command-type="scene_clear_all"]') + ).toHaveCount(1) }) test('if you click the format button it formats your code', async ({ diff --git a/src/wasm-lib/kcl/src/execution/mod.rs b/src/wasm-lib/kcl/src/execution/mod.rs index c48a71bbe..82378e4ed 100644 --- a/src/wasm-lib/kcl/src/execution/mod.rs +++ b/src/wasm-lib/kcl/src/execution/mod.rs @@ -43,6 +43,7 @@ use crate::{ settings::types::UnitLength, source_range::{ModuleId, SourceRange}, std::{args::Arg, StdLib}, + walk::Node as WalkNode, ExecError, Program, }; @@ -2002,9 +2003,12 @@ impl ExecutorContext { return None; } - let mut old_ast = old.ast.inner; + let mut old_ast = old.ast; + let mut new_ast = info.new_ast; + + // The digests should already be computed, but just in case we don't + // want to compare against none. old_ast.compute_digest(); - let mut new_ast = info.new_ast.inner.clone(); new_ast.compute_digest(); // Check if the digest is the same. @@ -2013,12 +2017,84 @@ impl ExecutorContext { } // Check if the changes were only to Non-code areas, like comments or whitespace. + Some(self.generate_changed_program(old_ast, new_ast)) + } - // For any unhandled cases just re-execute the whole thing. - Some(CacheResult { - clear_scene: true, - program: info.new_ast, - }) + /// Force-generate a new CacheResult, even if one shouldn't be made. The + /// way in which this gets invoked should always be through + /// [Self::get_changed_program]. This is purely to contain the logic on + /// how we construct a new [CacheResult]. + pub fn generate_changed_program(&self, old_ast: Node, new_ast: Node) -> CacheResult { + let mut generated_program = new_ast.clone(); + generated_program.body = vec![]; + + if !old_ast.body.iter().zip(new_ast.body.iter()).all(|(old, new)| { + let old_node: WalkNode = old.into(); + let new_node: WalkNode = new.into(); + old_node.digest() == new_node.digest() + }) { + // If any of the nodes are different in the stretch of body that + // overlaps, we have to bust cache and rebuild the scene. This + // means a single insertion or deletion will result in a cache + // bust. + + return CacheResult { + clear_scene: true, + program: new_ast, + }; + } + + // otherwise the overlapping section of the ast bodies matches. + // Let's see what the rest of the slice looks like. + + match new_ast.body.len().cmp(&old_ast.body.len()) { + std::cmp::Ordering::Less => { + // the new AST is shorter than the old AST -- statements + // were removed from the "current" code in the "new" code. + // + // Statements up until now match which means this is a + // "pure delete" of the remaining slice, when we get to + // supporting that. + + // Cache bust time. + CacheResult { + clear_scene: true, + program: new_ast, + } + } + std::cmp::Ordering::Greater => { + // the new AST is longer than the old AST, which means + // statements were added to the new code we haven't previously + // seen. + // + // Statements up until now are the same, which means this + // is a "pure addition" of the remaining slice. + + generated_program + .body + .extend_from_slice(&new_ast.body[old_ast.body.len()..]); + + CacheResult { + clear_scene: false, + program: generated_program, + } + } + std::cmp::Ordering::Equal => { + // currently unreachable, but lets pretend like the code + // above can do something meaningful here for when we get + // to diffing and yanking chunks of the program apart. + + // We don't actually want to do anything here; so we're going + // to not clear and do nothing. Is this wrong? I don't think + // so but i think many things. This def needs to change + // when the code above changes. + + CacheResult { + clear_scene: false, + program: generated_program, + } + } + } } /// Perform the execution of a program.