Skip to content

Commit

Permalink
Misc/gen2 sdk perf boost (#3519)
Browse files Browse the repository at this point in the history
## Description

- taking over #3485
- applies only to React, React Native and NextJS SDKs. Disabled for all
other SDKs due to various issues in reactivity.

---------

Co-authored-by: Steve Sewell <[email protected]>
  • Loading branch information
samijaber and steve8708 committed Sep 6, 2024
1 parent 1a446bc commit 1a69210
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 50 deletions.
7 changes: 7 additions & 0 deletions .changeset/new-shrimps-grab.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@builder.io/sdk-react-nextjs": patch
"@builder.io/sdk-react": patch
"@builder.io/sdk-react-native": patch
---

Chore: improve performance by caching block processing during renders.
38 changes: 32 additions & 6 deletions packages/sdks-tests/src/e2e-tests/large-reactive-state.spec.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,28 @@
import { expect } from '@playwright/test';
import { excludeGen2, excludeTestFor, test } from '../helpers/index.js';
import { excludeTestFor, test } from '../helpers/index.js';
import {
launchEmbedderAndWaitForSdk,
sendContentUpdateMessage,
sendPatchUpdatesMessage,
} from '../helpers/visual-editor.js';
import { LARGE_REACTIVE_STATE_CONTENT } from '../specs/large-reactive-state.js';
import type { Sdk } from '../helpers/sdk.js';

export const excludeSdksWithoutCachedProcessedBlock = (sdk: Sdk) =>
excludeTestFor(
{
svelte: true,
vue: true,
angular: true,
qwik: true,
solid: true,
},
sdk
);

test.describe('Large Reactive State', () => {
test('renders entire page correctly', async ({ page, sdk }) => {
test.skip(sdk === 'qwik', 'performance improvement not implemented yet');
test.skip(excludeSdksWithoutCachedProcessedBlock(sdk), 'Not implemented');
await page.goto('/large-reactive-state');

await expect(page.getByText('0', { exact: true })).toBeVisible();
Expand All @@ -18,7 +31,7 @@ test.describe('Large Reactive State', () => {

test('maintains reactivity with large state', async ({ page, sdk }) => {
test.fail(excludeTestFor({ rsc: true }, sdk));
test.skip(excludeGen2(sdk), 'performance improvement not implemented yet');
test.skip(excludeSdksWithoutCachedProcessedBlock(sdk), 'Not implemented');

await page.goto('/large-reactive-state');

Expand All @@ -34,8 +47,8 @@ test.describe('Large Reactive State', () => {

test('performance check for large state updates', async ({ page, sdk, packageName }) => {
test.fail(excludeTestFor({ rsc: true }, sdk));
test.skip(excludeGen2(sdk), 'performance improvement not implemented yet');
test.fail(packageName === 'gen1-remix', 'hydration mismatch');
test.skip(excludeSdksWithoutCachedProcessedBlock(sdk), 'Not implemented');

await page.goto('/large-reactive-state');

Expand All @@ -49,8 +62,14 @@ test.describe('Large Reactive State', () => {
const endTime = Date.now();
const duration = endTime - startTime;

const logMsg = `Large state updates duration: ${duration}ms`;
console.log(logMsg);
test.info().annotations.push({
type: 'performance',
description: logMsg,
});
// Assuming a threshold of 1000ms for 10 updates
expect(duration).toBeLessThan(5000);
expect(duration).toBeLessThan(10000);

// Verify final state
await expect(page.getByText('10', { exact: true })).toBeVisible();
Expand All @@ -67,7 +86,7 @@ test.describe('Large Reactive State', () => {
packageName === 'gen1-next' || packageName === 'gen1-remix',
'visual editing is only implemented for gen1 react-vite.'
);
test.skip(excludeGen2(sdk), 'performance improvement not implemented yet');
test.skip(excludeSdksWithoutCachedProcessedBlock(sdk), 'Not implemented');

await launchEmbedderAndWaitForSdk({
path: '/large-reactive-state-editing',
Expand Down Expand Up @@ -120,6 +139,13 @@ test.describe('Large Reactive State', () => {
const endTime = Date.now();
const duration = endTime - startTime;

const logMsg = `Visual editor updates duration: ${duration}ms`;
console.log(logMsg);
test.info().annotations.push({
type: 'performance',
description: logMsg,
});

expect(duration).toBeLessThan(10000);
});
});
12 changes: 1 addition & 11 deletions packages/sdks/src/components/block/block.helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,18 @@ import type {
} from '../../context/types.js';
import { evaluate } from '../../functions/evaluate/index.js';
import { extractTextStyles } from '../../functions/extract-text-styles.js';
import { getProcessedBlock } from '../../functions/get-processed-block.js';
import { getStyle } from '../../functions/get-style.js';
import type { BuilderBlock } from '../../types/builder-block.js';
import type { RepeatData } from './types.js';

export const getComponent = ({
block,
context,
registeredComponents,
}: {
block: BuilderBlock;
context: BuilderContextInterface;
registeredComponents: RegisteredComponents;
}) => {
const componentName = getProcessedBlock({
block,
localState: context.localState,
rootState: context.rootState,
rootSetState: context.rootSetState,
context: context.context,
shouldEvaluateBindings: false,
}).component?.name;
const componentName = block.component?.name;

if (!componentName) {
return null;
Expand Down
74 changes: 65 additions & 9 deletions packages/sdks/src/components/block/block.lite.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
For,
Show,
onMount,
onUpdate,
useMetadata,
useStore,
useTarget,
Expand All @@ -14,6 +15,7 @@ import type {
} from '../../context/types.js';
import { getBlockComponentOptions } from '../../functions/get-block-component-options.js';
import { getProcessedBlock } from '../../functions/get-processed-block.js';
import { isPreviewing } from '../../server-index.js';
import type { BuilderBlock } from '../../types/builder-block.js';
import DynamicDiv from '../dynamic-div.lite.jsx';
import { bindAnimations } from './animator.js';
Expand Down Expand Up @@ -56,21 +58,39 @@ useMetadata({

export default function Block(props: BlockProps) {
const state = useStore({
get blockComponent() {
return getComponent({
block: props.block,
context: props.context.value,
registeredComponents: props.registeredComponents,
});
},
get repeatItem() {
return getRepeatItemData({
block: props.block,
context: props.context.value,
});
},
/**
* Simple agnostic memoization for the processed block
* This is used to avoid re-processing the block on every render
* We need to make this a property on an object so setState() isn't
* called causing infinite rerenders e.g. in React
*/
_processedBlock: { value: null as BuilderBlock | null, update: false },
get processedBlock(): BuilderBlock {
return props.block.repeat?.collection
useTarget({
svelte: () => {},
vue: () => {},
angular: () => {},
qwik: () => {},
solid: () => {},

// @ts-expect-error: missing return value
default: () => {
if (
state._processedBlock.value &&
!state._processedBlock.update &&
!isPreviewing()
) {
return state._processedBlock.value;
}
},
});
const blockToUse = props.block.repeat?.collection
? props.block
: getProcessedBlock({
block: props.block,
Expand All @@ -80,6 +100,26 @@ export default function Block(props: BlockProps) {
context: props.context.value.context,
shouldEvaluateBindings: true,
});

useTarget({
svelte: () => {},
vue: () => {},
angular: () => {},
qwik: () => {},
solid: () => {},
default: () => {
state._processedBlock.value = blockToUse;
state._processedBlock.update = false;
},
});

return blockToUse;
},
get blockComponent() {
return getComponent({
block: state.processedBlock,
registeredComponents: props.registeredComponents,
});
},
get Tag() {
const shouldUseLink =
Expand Down Expand Up @@ -172,6 +212,22 @@ export default function Block(props: BlockProps) {
},
});

/**
* This trick forces the component to re-compute the `processedBlock` on every update.
*/
onUpdate(() => {
useTarget({
svelte: () => {},
vue: () => {},
angular: () => {},
qwik: () => {},
solid: () => {},
default: () => {
state._processedBlock.update = true;
},
});
});

onMount(() => {
useTarget({
reactNative: () => {},
Expand All @@ -192,7 +248,7 @@ export default function Block(props: BlockProps) {

return (
<Show when={state.canShowBlock}>
<BlockStyles block={props.block} context={props.context.value} />
<BlockStyles block={state.processedBlock} context={props.context.value} />
<Show
when={!state.blockComponent?.noWrap}
else={
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
import { TARGET } from '../../../constants/target.js';
import type { BuilderContextInterface } from '../../../context/types.js';
import { camelToKebabCase } from '../../../functions/camel-to-kebab-case.js';
import { getProcessedBlock } from '../../../functions/get-processed-block.js';
import { createCssClass } from '../../../helpers/css.js';
import { checkIsDefined } from '../../../helpers/nullable.js';
import type { BuilderBlock } from '../../../types/builder-block.js';
Expand All @@ -26,14 +25,7 @@ useMetadata({
export default function BlockStyles(props: BlockStylesProps) {
const state = useStore({
get canShowBlock() {
const processedBlock = getProcessedBlock({
block: props.block,
localState: props.context.localState,
rootState: props.context.rootState,
rootSetState: props.context.rootSetState,
context: props.context.context,
shouldEvaluateBindings: true,
});
const processedBlock = props.block;
// only render styles for blocks that are visible
if (checkIsDefined(processedBlock.hide)) {
return !processedBlock.hide;
Expand All @@ -45,14 +37,7 @@ export default function BlockStyles(props: BlockStylesProps) {
},

get css(): string {
const processedBlock = getProcessedBlock({
block: props.block,
localState: props.context.localState,
rootState: props.context.rootState,
rootSetState: props.context.rootSetState,
context: props.context.context,
shouldEvaluateBindings: true,
});
const processedBlock = props.block;

const styles = processedBlock.responsiveStyles;

Expand Down
16 changes: 15 additions & 1 deletion packages/sdks/src/functions/evaluate/evaluate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,20 @@ import { chooseBrowserOrServerEval } from './choose-eval.js';
import type { EvaluatorArgs, ExecutorArgs } from './helpers.js';
import { getBuilderGlobals, parseCode } from './helpers.js';

// Caching is dangerous for several reasons
// - JSON.stringify is not safe to run on things like `context`
// because anything can be passed, including non-stringifiable objects,
// circiular references, etc.
// - JSON.stringify is expensive to run as a cache key
// - This likely only helped because we were running processBlock multiple times
// but that is not fixed, and this will cause bugs and issues if enabled
//
// Perhaps the best approach is to allow users to configure if caching is enabled or not
// for instance on edge runtimes with slower evaluation times via the interpreter
// That said, the repeat processBLock calls was making any evaluation time 10x+ slower
// which is now fixed, so this still may not be necessary
const DISABLE_CACHE = true;

type EvalValue = unknown;

class EvalCache {
Expand Down Expand Up @@ -56,7 +70,7 @@ export function evaluate({
localState,
};

if (enableCache) {
if (enableCache && !DISABLE_CACHE) {
const cacheKey = EvalCache.getCacheKey(args);
const cachedValue = EvalCache.getCachedValue(cacheKey);

Expand Down
Loading

0 comments on commit 1a69210

Please sign in to comment.