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

Optimizations 1 #3377

Draft
wants to merge 17 commits into
base: v11
Choose a base branch
from
Draft

Optimizations 1 #3377

wants to merge 17 commits into from

Conversation

developit
Copy link
Member

@developit developit commented Dec 15, 2021

  • Always pass parameter values to internal functions
  • Rename internal.rerender() to internal.render() (gzip optimizations, since "render" already appears in source)
  • Turn enqueueRender() into the implementation of internal.render()
    • It now uses this to access the Internal instance to be enqueued.
    • Every place that was accessing enqueueRender() is now instead calling internal.render(), which removes the direct dependency on that function and improves tree-shaking
    • Component no longer accesses the renderer, and the renderer no longer accesses Component (yay!)
  • slight simplification for createContext to reduce size
  • Allow passing an existing root Internal to createRoot()
    • simplifies the implementation of our render() and hydrate() exported functions
  • Rename the functions and render queue to reflect what they do
    • rerenderQueue --> renderQueue (not always repeated renders, also I type "rerender" wrong every time)
    • process() --> processRenderQueue()
  • Remove createVNode() and use createElement() everywhere instead
    • we still get the single-callsite benefit from createVNode because we're always using one function to allocate VNodes
    • createElement has a fast-path optimization for 3 or fewer arguments that we always hit in internal usage
  • minify .flags to .f (-23b)

@github-actions
Copy link

github-actions bot commented Dec 15, 2021

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -1% - +1% (-0.73ms - +0.69ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: unsure 🔍 -3% - +3% (-2.79ms - +2.78ms)
    preact-local vs preact-master
  • 07_create10k: unsure 🔍 -1% - +1% (-7.52ms - +5.74ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -2% - +1% (-3.61ms - +1.53ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -3% - +0% (-2.48ms - +0.38ms)
    preact-local vs preact-master
  • many_updates: unsure 🔍 -2% - +3% (-2.93ms - +3.44ms)
    preact-local vs preact-master
  • text_update: unsure 🔍 -0% - +3% (-0.19ms - +2.32ms)
    preact-local vs preact-master
  • todo: unsure 🔍 -2% - +1% (-1.15ms - +0.56ms)
    preact-local vs preact-master

usedJSHeapSize

  • 02_replace1k: slower ❌ 1% - 1% (0.04ms - 0.06ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: slower ❌ 1% - 1% (0.04ms - 0.05ms)
    preact-local vs preact-master
  • 07_create10k: slower ❌ 2% - 2% (0.44ms - 0.44ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -1% - +3% (-0.01ms - +0.05ms)
    preact-local vs preact-master
  • hydrate1k: slower ❌ 1% - 1% (0.09ms - 0.09ms)
    preact-local vs preact-master
  • many_updates: slower ❌ 0% - 2% (0.03ms - 0.09ms)
    preact-local vs preact-master
  • text_update: slower ❌ 6% - 9% (0.06ms - 0.09ms)
    preact-local vs preact-master
  • todo: unsure 🔍 -2% - +1% (-0.02ms - +0.01ms)
    preact-local vs preact-master

Results

02_replace1k

duration

VersionAvg timevs preact-mastervs preact-local
preact-master103.58ms - 104.82ms-unsure 🔍
-1% - +1%
-0.69ms - +0.73ms
preact-local103.84ms - 104.52msunsure 🔍
-1% - +1%
-0.73ms - +0.69ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master4.01ms - 4.02ms-faster ✔
1% - 1%
0.04ms - 0.06ms
preact-local4.06ms - 4.07msslower ❌
1% - 1%
0.04ms - 0.06ms
-

run-warmup-0

VersionAvg timevs preact-mastervs preact-local
preact-master48.47ms - 49.17ms-unsure 🔍
-2% - +0%
-0.83ms - +0.10ms
preact-local48.88ms - 49.50msunsure 🔍
-0% - +2%
-0.10ms - +0.83ms
-

run-warmup-1

VersionAvg timevs preact-mastervs preact-local
preact-master58.79ms - 60.61ms-unsure 🔍
-1% - +3%
-0.70ms - +2.01ms
preact-local58.04ms - 60.05msunsure 🔍
-3% - +1%
-2.01ms - +0.70ms
-

run-warmup-2

VersionAvg timevs preact-mastervs preact-local
preact-master52.02ms - 52.73ms-unsure 🔍
-0% - +2%
-0.01ms - +1.00ms
preact-local51.52ms - 52.24msunsure 🔍
-2% - +0%
-1.00ms - +0.01ms
-

run-warmup-3

VersionAvg timevs preact-mastervs preact-local
preact-master40.81ms - 41.80ms-faster ✔
2% - 5%
0.65ms - 2.25ms
preact-local42.12ms - 43.38msslower ❌
2% - 5%
0.65ms - 2.25ms
-

run-warmup-4

VersionAvg timevs preact-mastervs preact-local
preact-master53.45ms - 55.90ms-unsure 🔍
-2% - +4%
-1.35ms - +1.92ms
preact-local53.31ms - 55.47msunsure 🔍
-4% - +2%
-1.92ms - +1.35ms
-

run-final

VersionAvg timevs preact-mastervs preact-local
preact-master103.61ms - 104.85ms-unsure 🔍
-1% - +1%
-0.67ms - +0.75ms
preact-local103.85ms - 104.53msunsure 🔍
-1% - +1%
-0.75ms - +0.67ms
-
03_update10th1k_x16

duration

VersionAvg timevs preact-mastervs preact-local
preact-master97.73ms - 101.73ms-unsure 🔍
-3% - +3%
-2.78ms - +2.79ms
preact-local97.79ms - 101.66msunsure 🔍
-3% - +3%
-2.79ms - +2.78ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master4.00ms - 4.01ms-faster ✔
1% - 1%
0.04ms - 0.05ms
preact-local4.04ms - 4.05msslower ❌
1% - 1%
0.04ms - 0.05ms
-
07_create10k

duration

VersionAvg timevs preact-mastervs preact-local
preact-master1136.05ms - 1145.61ms-unsure 🔍
-1% - +1%
-5.74ms - +7.52ms
preact-local1135.35ms - 1144.54msunsure 🔍
-1% - +1%
-7.52ms - +5.74ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master28.95ms - 28.96ms-faster ✔
1% - 2%
0.44ms - 0.44ms
preact-local29.40ms - 29.40msslower ❌
2% - 2%
0.44ms - 0.44ms
-
filter_list

duration

VersionAvg timevs preact-mastervs preact-local
preact-master169.23ms - 172.39ms-unsure 🔍
-1% - +2%
-1.53ms - +3.61ms
preact-local167.74ms - 171.80msunsure 🔍
-2% - +1%
-3.61ms - +1.53ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master1.87ms - 1.91ms-unsure 🔍
-2% - +1%
-0.05ms - +0.01ms
preact-local1.88ms - 1.93msunsure 🔍
-1% - +3%
-0.01ms - +0.05ms
-
hydrate1k

duration

VersionAvg timevs preact-mastervs preact-local
preact-master76.37ms - 78.65ms-unsure 🔍
-1% - +3%
-0.38ms - +2.48ms
preact-local75.59ms - 77.33msunsure 🔍
-3% - +0%
-2.48ms - +0.38ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master6.73ms - 6.73ms-faster ✔
1% - 1%
0.09ms - 0.09ms
preact-local6.82ms - 6.83msslower ❌
1% - 1%
0.09ms - 0.09ms
-
many_updates

duration

VersionAvg timevs preact-mastervs preact-local
preact-master131.78ms - 136.16ms-unsure 🔍
-3% - +2%
-3.44ms - +2.93ms
preact-local131.92ms - 136.53msunsure 🔍
-2% - +3%
-2.93ms - +3.44ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master5.61ms - 5.66ms-faster ✔
0% - 2%
0.03ms - 0.09ms
preact-local5.67ms - 5.71msslower ❌
0% - 2%
0.03ms - 0.09ms
-
text_update

duration

VersionAvg timevs preact-mastervs preact-local
preact-master74.20ms - 76.03ms-unsure 🔍
-3% - +0%
-2.32ms - +0.19ms
preact-local75.33ms - 77.04msunsure 🔍
-0% - +3%
-0.19ms - +2.32ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master1.01ms - 1.02ms-faster ✔
5% - 8%
0.06ms - 0.09ms
preact-local1.07ms - 1.10msslower ❌
6% - 9%
0.06ms - 0.09ms
-
todo

duration

VersionAvg timevs preact-mastervs preact-local
preact-master48.48ms - 50.13ms-unsure 🔍
-1% - +2%
-0.56ms - +1.15ms
preact-local48.79ms - 49.22msunsure 🔍
-2% - +1%
-1.15ms - +0.56ms
-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-local
preact-master1.04ms - 1.06ms-unsure 🔍
-1% - +2%
-0.01ms - +0.02ms
preact-local1.04ms - 1.05msunsure 🔍
-2% - +1%
-0.02ms - +0.01ms
-

tachometer-reporter-action v2 for Benchmarks

@coveralls
Copy link

coveralls commented Dec 15, 2021

Coverage Status

Coverage decreased (-0.004%) to 99.692% when pulling 6d7f441 on wip-optimizations-v11 into 2ce0eaa on main.

@github-actions
Copy link

github-actions bot commented Dec 15, 2021

Size Change: -334 B (1%)

Total Size: 32.4 kB

Filename Size Change
compat/dist/compat.js 3.69 kB -3 B (0%)
compat/dist/compat.umd.js 3.75 kB -5 B (0%)
debug/dist/debug.js 3.2 kB -3 B (0%)
debug/dist/debug.umd.js 3.3 kB -3 B (0%)
dist/preact.js 4.51 kB -73 B (1%)
dist/preact.min.js 4.55 kB -72 B (1%)
dist/preact.umd.js 4.61 kB -70 B (1%)
hooks/dist/hooks.js 1.26 kB -7 B (0%)
hooks/dist/hooks.umd.js 1.33 kB -6 B (0%)
jsx-runtime/dist/jsxRuntime.js 296 B -46 B (15%) 👏
jsx-runtime/dist/jsxRuntime.umd.js 379 B -46 B (12%) 👏
ℹ️ View Unchanged
Filename Size Change
devtools/dist/devtools.js 232 B 0 B
devtools/dist/devtools.umd.js 316 B 0 B
test-utils/dist/testUtils.js 431 B 0 B
test-utils/dist/testUtils.umd.js 516 B 0 B

compressed-size-action

@@ -34,6 +34,7 @@
"props": {
"cname": 6,
"props": {
"$flags": "f",
Copy link
Member

Choose a reason for hiding this comment

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

I would opt to keep this to flags to encourage use of library authors for additional extensions that we don't want to support first-party

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been going back and forth on it. It pains me that this is 23b just for lags 👁

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the community being able to use this without looking at this file kind off boats well for me but yeah, one opinion 😅 also the types won't ever reflect .f unless we start doing two different type files

Copy link
Member Author

Choose a reason for hiding this comment

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

true about types too (though that makes me think we should probably already be doing internal VS external files given we have other minified properties)

src/component.js Outdated
commitRoot(commitQueue, internal);
}
}
const renderDirtyInternal = internal => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe rename this to renderInternal :p

Copy link
Member Author

Choose a reason for hiding this comment

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

renderEnqueuedInternal?

Copy link
Member

Choose a reason for hiding this comment

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

Yes something like that, it's a nit, the dirty part feels a bit odd 😅

}
// re-render subscribers in response to value change
else if (props.value !== this._prev) {
this._subs.forEach(enqueueRender);
this._subs.forEach(i => i.render());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this._subs.forEach(i => i.render());
this._subs.forEach(i => { i.render() });

Saves some bytes

Copy link
Member Author

Choose a reason for hiding this comment

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

ha! can't believe I missed that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JoviDeCroock would you believe that actually adds 3b?! gzip

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it comes from the return keyword added at build time 🤷

src/create-element.js Outdated Show resolved Hide resolved
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.

None yet

4 participants