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

Fix hook expr optimization and add optimization steps #20

Merged
merged 19 commits into from
May 14, 2023

Conversation

lxsmnsyc
Copy link
Owner

@lxsmnsyc lxsmnsyc commented May 13, 2023

is-contains-hook was proved to be problematic in some scenarios (e.g. useEffect(() => ..., [useA()]) so this PR changes it so that:

  • hook calls are guaranteed to be on the function body.
  • hook calls are never memoized
  • hook calls don't remain in place.
  • hooks can be partially optimized

Some minor fixes:

  • Fix constant check on hook calls
    • previously it only returns false if the hook's scope binding exists however this isn't the expected case if the hook function is globally declared.
  • Fix object expression to memoize the key first before the value (this is just to follow the evaluation order properly)
  • Assignment expressions are now hoisted.

Edit:
This PR now also adds 3 new optimization steps:

  • pre-inlining
    • Expressions that can be inlined are now inlined. This allows the memoization step to save more slots since expressions can now be merged as a whole rather than separately.
  • expanding
    • Hook calls and assignment expressions are hoisted back to the function body since they cannot be inlined due to memoization conditions. We must guarantee hooks are called w/o conditions.
  • memoization
    • The core step
  • post-inlining
    • Like pre-inlining. This allows the compiler to remove potentially most of the variable declarations made by the memoization step since most of it can be inlined safely due to having single references.

Here's an example code that has been optimized by the new step:

function Example() {
  const a = Math.random();
  const b = Math.random();
  console.log(a + b);
}
// becomes
import { useMemo as _useMemo } from "react";
import { $$cache as _$$cache } from "forgetti/runtime";
function Example() {
  let _c = _$$cache(_useMemo, 1);
  0 in _c ? _c[0] : _c[0] = console.log(Math.random() + Math.random());
}

@lxsmnsyc lxsmnsyc changed the base branch from main to next May 13, 2023 11:31
@lxsmnsyc
Copy link
Owner Author

lxsmnsyc commented May 13, 2023

@SukkaW I'm sorry I had to remove the check (although it is likely to be removed in the future, but I just found out a simpler solution).

Initially I was theorizing the optimization process was divided in three steps:

  • Unfolding (hoisting all call expressions into the function body in the form of variables. This is problematic because we can no longer progressively check if parts of the expression or the entirety of it is a single constant.)
  • Optimization (the main process)
  • Sweep and merge (removing all unused values, merging all expressions that only has a single dependendy (e.g. A -> B -> C is flattened so that there's only one memoization).

It seems that step 1 is unnecessary but step 3 seems to be more likely, however I don't see it achieving yet so I had to make sure that the hook problem is solved as early as possible (the 3 step optimization is supposed to solve it).

@SukkaW
Copy link
Contributor

SukkaW commented May 13, 2023

@SukkaW I'm sorry I had to remove the check (although it is likely to be removed in the future, but I just found out a simpler solution).

Initially I was theorizing the optimization process was divided in three steps:

  • Unfolding (hoisting all call expressions into the function body in the form of variables. This is problematic because we can no longer progressively check if parts of the expression or the entirety of it is a single constant.)
  • Optimization (the main process)
  • Sweep and merge (removing all unused values, merging all expressions that only has a single dependendy (e.g. A -> B -> C is flattened so that there's only one memoization).

It seems that step 1 is unnecessary but step 3 seems to be more likely, however I don't see it achieving yet so I had to make sure that the hook problem is solved as early as possible (the 3 step optimization is supposed to solve it).

Awesome! This sounds better than the current check implementation.

In the mean time, there might be more edge cases to cover. I can start using forgetti in more of my side projects to discover them.

@lxsmnsyc
Copy link
Owner Author

Okay I've decided to add the optimization steps.

@lxsmnsyc lxsmnsyc changed the title Fix hook expr optimization Fix hook expr optimization and add optimization steps May 14, 2023
@lxsmnsyc lxsmnsyc merged commit 3245fe1 into next May 14, 2023
@lxsmnsyc lxsmnsyc deleted the fix-hook-expr-optimization branch May 14, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants