winglang / wing

A programming language for the cloud ☁️ A unified programming model, combining infrastructure and runtime code into one language ⚡
https://winglang.io
Other
4.77k stars 189 forks source link

Iterations are very slow for bigger projects #6540

Closed eladb closed 1 month ago

eladb commented 1 month ago

I tried this:

Clone this: https://github.com/winglang/translator-example

Run: wing it

Make any update (e.g. change index.html).

This happened:

The simulator always updates 10 resources.

root/Default/input_bucket/oncreate/Policy stopped
root/Default/input_bucket/oncreate/TopicEventMapping0 stopped
root/Default/input_bucket/oncreate/OnMessage0 stopped
root/Default/Translators/translator_Hebrew/Queue/QueueEventMapping0 stopped
root/Default/Translators/translator_Hebrew/Queue/Policy stopped
root/Default/Translators/translator_Hebrew/Queue/Consumer0 stopped
root/Default/Translators/translator_Italian/Queue/QueueEventMapping0 stopped
root/Default/Translators/translator_Italian/Queue/Policy stopped
root/Default/Translators/translator_Italian/Queue/Consumer0 stopped
root/Default/Translators/translator_Chinese/Queue/QueueEventMapping0 stopped
root/Default/Translators/translator_Chinese/Queue/Policy stopped
root/Default/Translators/translator_Chinese/Queue/Consumer0 stopped
root/Default/Translators/translator_Klingon/Queue/QueueEventMapping0 stopped
root/Default/Translators/translator_Klingon/Queue/Policy stopped
root/Default/Translators/translator_Klingon/Queue/Consumer0 stopped
root/Default/Api/Api/Policy stopped
root/Default/Api/Api/ApiEventMapping0 stopped
root/Default/Api/Api/OnRequestHandler0 stopped
root/Default/Api/Api/ApiEventMapping1 stopped
root/Default/Api/Api/OnRequestHandler1 stopped
root/Default/Api/Api/ApiEventMapping2 stopped
root/Default/Api/Api/OnRequestHandler2 stopped
root/Default/Tools/translate this stopped
root/Default/Tools/redrive stopped

I expected this:

I expected minimal updates. I think this is related to the fact that we are unable to invalidate inflight closures. Let's see what we need to do to fix this because in a project that's slightly bigger than a toy, the iteration cycle becomes very slow.

Is there a workaround?

No response

Anything else?

No response

Wing Version

No response

Node.js Version

No response

Platform(s)

No response

Community Notes

eladb commented 1 month ago

Thinking out loud: I understand the issue is invalidating the inflight closures based on dependencies. What's the cheapest way to get a list of required local files (recursive) and libraries (non recursive)?

Chriscbr commented 1 month ago

I had an idea for updating the plan method for cloud.Function so that it does something like this (in pseudocode):

plan(newConfig) {
  if newConfig != currentConfig {
    return UpdatePlan.REPLACE;
  }

  let files = ... // get a list of files that were included in the last bundle
  let modified = // check if any of the files were modified since the time the last bundle was created
  if (modified) {
    return UpdatePlan.REPLACE;
  }

  // none of the files were changed, so we probably don't need to re-bundle anything
  return UpdatePlan.SKIP;
}

The concept is if all of the Function's configuration is the same, and none of the files that were bundled have been modified, we can probably skip the Function replacement. This isn't fool-proof: even if all of the bundled files haven't changed, it's still possible for esbuild to produce a different bundle (for example, if a new file was introduced to the file system). But it might be good enough for our purposes.


But while implementing this I discovered what seems like a problem with the update algorithm used in the simulator (from https://github.com/winglang/wing/pull/5821). The problem causes the simulator to always perform unneeded replacements for cloud.Function as long as they depend on other resources. To understand the problem, suppose we have an app with two resources. You can imagine their static configuration is like this:

a_instance:
  type: A
  props:
    prop1: "foo"

b_instance:
  type: B
  props:
    prop1: "${wsim#a_instance#attrs.attr1}"

We can see b_instance's configuration implicitly refers to a deploy-time attribute of a_instance. When the app runs, the resources are deployed one after the other, a_instance computes attr1, and the models in the simulator are updated to something like:

a_instance:
  type: A
  props:
    prop1: "foo"
  attrs:
    attr1: "foofoo"

b_instance:
  type: B
  props:
    prop1: "foofoo"

Suppose the user updates their app, and now a_instance's prop has a different value:

a_instance:
  type: A
  props:
    prop1: "bar"

a_instance might decide that "bar" is such a different value that the instance needs to be fully replaced. That's OK.

But how are we calculating whether b_instance needs to be replaced or not? During the planning phase, we have the resolved configuration of the currently running app, but only the static / unresolved configuration of the new app, so they can't be directly compared. It wouldn't make sense to only compare the old static configuration to the new static configuration either, since the resolved values might differ.

A naive idea is to try resolving the new configuration - but we're just in the "planning" phase, so we don't actually know the new deploy-time attributes of a_instance (which means we can't resolve the configuration of b_instance either).

Another idea for a solution could be to resolve all of the configuration, but add some kind of way to flag values that will only be known after the update is performed:

a_instance:
  type: A
  props:
    prop1: "foo"
  attrs:
    attr1: "(known after update)"

b_instance:
  type: B
  props:
    prop1: "(known after update)"
eladb commented 1 month ago

I am not 100% sure I understand the issue. If one of the props of a_instance have changed, it will be replaced and then b_instance will also be updated because it depends on a_instance (we don't have an "in place update", just replacements).

Chriscbr commented 1 month ago

and then b_instance will also be updated because it depends on a_instance

How is b_instance deciding whether it should be updated or not? There's a planUpdate function which calls plan() on the currently running simulation of b_instance, but it's receiving an unresolved version of the configuration (containing tokens) even though it was constructed with props containing real values.

Chriscbr commented 1 month ago

From B's perspective, the currently running instance knows it was constructed with { prop1: "foofoo" }, and then someone called "plan" on it, passing { prop1: "" }. It's not B's responsibility to resolve tokens - that's the simulator's job.

eladb commented 1 month ago

Since b depends on a and a is updated, b should also be invalidated I believe.

Chriscbr commented 1 month ago

The issue is easier to see when you compare the props the instance is constructed with to the props passed to the plan method:

diff --git a/libs/wingsdk/src/target-sim/bucket.inflight.ts b/libs/wingsdk/src/target-sim/bucket.inflight.ts
index 884bbcf39..f4cf650e9 100644
--- a/libs/wingsdk/src/target-sim/bucket.inflight.ts
+++ b/libs/wingsdk/src/target-sim/bucket.inflight.ts
@@ -19,6 +19,7 @@ import {
 } from "../cloud";
 import { deserialize, serialize } from "../simulator/serialization";
 import {
+  BaseResourceSchema,
   ISimulatorContext,
   ISimulatorResourceInstance,
   UpdatePlan,
@@ -35,7 +36,8 @@ export class Bucket implements IBucketClient, ISimulatorResourceInstance {
   private readonly topicHandlers: Partial<Record<BucketEventType, string>>;
   private _metadata: Map<string, ObjectMetadata>;

-  public constructor(props: BucketSchema) {
+  public constructor(private readonly props: BucketSchema) {
+    console.error("constructed props", props);
     this.initialObjects = props.initialObjects ?? {};
     this._public = props.public ?? false;
     this.topicHandlers = props.topics;
@@ -91,8 +93,13 @@ export class Bucket implements IBucketClient, ISimulatorResourceInstance {

   public async cleanup(): Promise<void> {}

-  public async plan() {
-    return UpdatePlan.AUTO;
+  public async plan(newConfig: BaseResourceSchema) {
+    console.error("new props", newConfig.props);
+    // in theory, this is what UpdatePlan.AUTO does
+    if (JSON.stringify(newConfig.props) !== JSON.stringify(this.props)) {
+      return UpdatePlan.REPLACE;
+    } else {
+      return UpdatePlan.SKIP;
+    }
   }

   public async save(): Promise<void> {

When I start up the simulator with an app like this:

let api = new cloud.Api();
let b = new cloud.Bucket();
b.addObject("config.txt", api.url);

It logs the props as you might expect:

constructed props {
  public: false,
  initialObjects: { 'config.txt': 'http://127.0.0.1:50147' },
  topics: {}
}

But if I change the app (add cors: true or something like that) -- in a way that shouldn't cause the bucket to be destroyed -- the plan() call passes the props still with a token inside! So the bucket can't correctly figure out whether or not it should return REPLACE or SKIP. It's just going to return REPLACE all the time in this case, which would be a bug.

new props {
  public: false,
  initialObjects: { 'config.txt': '${wsim#root/Default/Api#attrs.url}' },
  topics: {}
}
eladb commented 1 month ago

I see, so this is not a case where an update should happen but skipped. It's the other way around.

The bug is that any resource that depends on another resource will eagerly be replaced even if the dependent hasn't changed because the token is always different than the resolved value.

I am wondering if it makes more sense to always pass in the token and not the resolved runtime value because the dependent will be always replaced anyway if the dependency is replaced. No?

Chriscbr commented 1 month ago

The bug is that any resource that depends on another resource will eagerly be replaced even if the dependent hasn't changed because the token is always different than the resolved value.

Bingo.

I am wondering if it makes more sense to always pass in the token and not the resolved runtime value because the dependent will be always replaced anyway if the dependency is replaced. No?

I'm not sure I understand your idea. We're already passing unresolved / token-ful config to the plan() method (maybe you're talking about passing in a token somewhere else?)

Since b depends on a and a is updated, b should also be invalidated I believe.

If b only depends on one field of a (could be a static property or a deploy-time attribute), and that field doesn't change but something else on a does change, then I don't think we have to force b to be replaced.

Keep in mind, if b needs access to the live instance of a (i.e. to make method calls to it etc.), then one way or another its configuration needs to take a dependency on the handle attribute of a. A resource always receives a fresh handle when it's replaced, so in those circumstances b would also have to get replaced.

monadabot commented 1 month ago

Congrats! :rocket: This was released in Wing 0.74.21.