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

Optimise parsed AST #3766

Closed
wants to merge 4 commits into from

Conversation

tanhauhau
Copy link
Member

@tanhauhau tanhauhau commented Oct 22, 2019

I see a few issues related on optimisation at AST level, below I'm showing a PoC for merging consecutive text-a-like nodes

It involves another tree walk on the ast.html to optimise the AST

@Rich-Harris what do you think?

Related issues:

TODO:

  • node.loc for newly created node.

@tanhauhau tanhauhau force-pushed the tanhauhau/optimise-build branch 3 times, most recently from 56c4a29 to 56f4ff9 Compare October 24, 2019 02:22
@Rich-Harris
Copy link
Member

In principle I'm definitely in favour of this. In the specific example in this PR I wonder if we should first solve the problem where we're duplicating the string concatenation:

+const get_t_value = ctx => `\n\tClicked ${ctx.count} ${ctx.count === 1 ? "time" : "times"}\n`;
+
function create_fragment(ctx) {
  let button;
-  let t_value = "\n\tClicked " + ctx.count + " " + (ctx.count === 1 ? "time" : "times") + "\n" + "";
+  let t_value = get_t_value(ctx);
  let t;
  let dispose;

  return {
    c() {
      button = element("button");
      t = text(t_value);
      dispose = listen(button, "click", ctx.increment);
    },
    m(target, anchor) {
      insert(target, button, anchor);
      append(button, t);
    },
    p(changed, ctx) {
-      if (changed.count && t_value !== (t_value = "\n\tClicked " + ctx.count + " " + (ctx.count === 1 ? "time" : "times") + "\n" + "")) set_data(t, t_value);
+      if (changed.count && t_value !== (t_value = get_t_value(ctx))) set_data(t, t_value);
    },
    i: noop,
    o: noop,
    d(detaching) {
      if (detaching) detach(button);
      dispose();
    }
  };
}

Of course, in some cases that would result in more code, plus there's the function call overhead... but if we can differentiate between cases where this approach makes sense and cases where it's better just to do everything inline, I reckon it's probably worth it.

I took the generated code from the new test and compared the minified results:

  • Without this PR — 649 bytes
  • With this PR — 622 bytes
  • With the diff above — 578 bytes

@tanhauhau
Copy link
Member Author

tanhauhau commented Oct 25, 2019

I thought of this before, I was wondering whether the following heuristics to determine whether or not to extract out as a function make sense?

const length_of_function = ('const get_t_value = ctx => ').length
const length_of_function_call = ('get_t_value(ctx)').length
const length_of_new_value_computation = ('"\n\tClicked " + ctx.count + " " + (ctx.count === 1 ? "time" : "times") + "\n" + ""').length


if (length_of_function + length_of_new_value_computation + length_of_function_call * 2 < length_of_new_value_computation * 2) {
  // extract out into a function
}

// or simplified

if (length_of_function + length_of_function_call * 2 < length_of_new_value_computation) {
  // extract out into a function
}

and of course length_of_function and length_of_function_call * 2 can be a heuristic constant, assuming get_t_value get minified, to be ~ 25 in total, so can further simplify to something like:

const HEURISTIC_LIMIT_TO_EXTRACT = 25;
if (length_of_new_value_computation > HEURISTIC_LIMIT_TO_EXTRACT) {
  // extract out into a function
}

and this shouldn't just for string concatenation, but rendering for any Expression

what do you think?

@Rich-Harris
Copy link
Member

Something like that seems about right, yeah. Although we should be comparing minified sizes, i.e. the length of const a=b=> rather than const get_t_value = ctx => , etc

@ghost
Copy link

ghost commented Oct 28, 2019

I think most modern browser VS engines will take care of optimizing out the function calls anyway so heuristics is best left to the interpreters. In fact you can see this behavior in a lot of the V8 dev talks where do deep dives into the optimization engines. I'd personally stick as many things into simple functions as possible because that is the unit of optimization that all browsers work with.

@tanhauhau
Copy link
Member Author

i realise a few things when fixing the tests:

  • this.node.should_cache is kind of a good heuristic to determine whether to extract out the expression computation as a function call
  • merging 2 nodes, (usually text node + expression tag, eg: text {expression}) end up bloating the output file
    • previously will only update the expression tag on change, now it needs to check whether the resultant new expression, eg: 'text' + expression has changed, and also need to compute the new expression, instead of just replacing the data.

@tanhauhau
Copy link
Member Author

i stumbled upon this issue, and wonder maybe insertAdjacentText might be a better alternative for this scenario:

function create_fragment(ctx) {
  let button;
-  let t_value = "\n\tClicked " + ctx.count + " " + (ctx.count === 1 ? "time" : "times") + "\n" + "";
- let t;
+  let t;
+  let t2;
  let dispose;

  return {
    c() {
      button = element("button");
-       t = text(t_value);
+      t = text(ctx.count);
+      t2 = text(ctx.count === 1 ? "time" : "times");
      dispose = listen(button, "click", ctx.increment);
    },
    m(target, anchor) {
      insert(target, button, anchor);
-       append(button, t);     
+      insertAdjacentText(button, "Clicked ");
+      append(button, t);
+      insertAdjacentText(button, " ");      
+      append(button, t2);
    },
    p(changed, ctx) {
+      if (changed.count) set_data(t, ctx.count);
+      if (changed.count) set_data(t2, ctx.count === 1 ? "time" : "times");
-       if (changed.count && t_value !== (t_value = "\n\tClicked " + ctx.count + " " + (ctx.count === 1 ? "time" : "times") + "\n" + "")) set_data(t, t_value);
    },
    i: noop,
    o: noop,
    d(detaching) {
      if (detaching) detach(button);
      dispose();
    }
  };
}

@tanhauhau
Copy link
Member Author

I've tried insertAdjacentText and insertAdjacentHtml, and tried it on the realworld app.

it just got 0.8% smaller in the js non-hydratable output 😞

@carpben
Copy link

carpben commented Nov 12, 2019

@tanhauhau , Suggestion in #3760 was implemented and the result was just 0.8% smaller? Can you share the code for the compiled JS?

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the right direction — 0.8% doesn't sound like much but it's definitely not nothing. Added a couple of comments/suggestions

@@ -25,7 +25,7 @@ function create_fragment(ctx) {
return {
c() {
p = element("p");
t0 = text(ctx.foo);
t0 = text(ctx.foo + "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be good if we didn't have the + "" here, since it's coerced to a string anyway

@@ -19,18 +19,16 @@ const file = undefined;

function create_fragment(ctx) {
let h1;
let t0_fn = ctx => `Hello ${ctx.name}!`;
let t0_value = t0_fn(ctx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be able to hoist these functions out of the block altogether, I think?

(I still have an ambition to merge the create/update phases so that we can just 'update' everything to its initial state, rather than having this duplication at all. But that's a future nice-to-have)

let t1;
let t2;

let t_fn = ctx => `
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should replace consecutive whitespace characters with a single space, both for consistency with the current behaviour and for reduced bytesize

@carpben
Copy link

carpben commented Nov 17, 2019

Here is a compiled update function from optimise-ast/expected.js:

p(changed, ctx) {
	if (changed.count && t_value !== (t_value = t_fn(ctx))) set_data(t, t_value);
},

Perhaps it could be further shortened.

  1. set_data already takes care of checking if the value of the updated string is equal to the value of the string node.
export function set_data(text, data) {
	data = '' + data;
	if (text.data !== data) text.data = data;
}

What purpose does checking it in the update function provide? Can we shorten it to:

p(changed, ctx) {
	if (changed.count) set_data(t, t_value = t_fn(ctx));
},
  1. Till now t_value was necessary for change comparison
if (changed.count && t_value !== (t_value = t_fn(ctx)))

However, if suggestion 1 could be implemented, we can let go of t_value altogether.

function create_fragment(ctx) {
	let button;

	let t_fn = ctx => `
	Clicked ${ctx.count} ${ctx.count === 1 ? "time" : "times"}
`;

-	let t_value = t_fn(ctx);
	let t;
	let dispose;

	return {
		c() {
			button = element("button");
-			t = text(t_value);
+			t = text(t_fn(ctx));
			dispose = listen(button, "click", ctx.increment);
		},
		m(target, anchor) {
			insert(target, button, anchor);
			append(button, t);
		},
		p(changed, ctx) {
-			if (changed.count && t_value !== (t_value = t_fn(ctx))) set_data(t, t_value);
+                       if (changed.count) set_data(t, t_fn(ctx));
		},
		i: noop,
		o: noop,
		d(detaching) {
			if (detaching) detach(button);
			dispose();
		}
	};
}

@Rich-Harris @tanhauhau

@dummdidumm
Copy link
Member

One request from the language tools perspective: If this takes a noticable amount of time to traverse, please do this step in the compilation step or at least add a flag that is turned on by default so language tools can turn it off for performance reasons.

@dummdidumm dummdidumm added this to the one day milestone Feb 22, 2023
@dummdidumm
Copy link
Member

This has a lot of merge conflicts (though most probably only related to tests, which can be fixed by not adding that + ""). That and the fact it's still very much WIP makes me think we should either close or keep open but only for illustrative purpose to have inspiration for when we actually want to do this.

@tanhauhau
Copy link
Member Author

yea i dont think i wanna rebase this, it was built on top of a very old commit

@tanhauhau tanhauhau closed this Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants