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.

Sign up for GitHub

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

Jump to bottom

Do typeof string check before looking for String constructor #4198

Merged
merged 3 commits into from
Nov 8, 2023
Merged

Do typeof string check before looking for String constructor #4198

merged 3 commits into from
Nov 8, 2023

Conversation

andrewiggins
Copy link
Member

@andrewiggins andrewiggins commented Nov 7, 2023 •
edited
Loading

In constructNewChildrenArray we look for the String constructor to support #4151 (i.e. strings created with new String). However, doing this check first leads to a megamorphic deopt in constructorNewChildrenArray since V8 has many different internal types for strings, leading to a megamorphic access for childVNode.constructor in the common case (normal strings and VNodes).

This PR adds back the typeof childVNode == 'string' check to catch normal strings first before falling back to childVNode.constructor == String to check for manually constructed strings.

The below screenshot shows the results of DeoptExplorer running many_updates benchmark on main. We hit a deopt on the childVNode.constructor due to the number of maps it hits (V8 has many different internal string representations leading to different maps at this location).

image

Adding back the typeof childVNode == 'string' removes the deopt for the common case:

image

To reduce size impact I've included the following golfs:

  • Store matching oldVNode in variable in constructNewChildrenArray (-4 B)
  • Inline insertion checks into skew block (-3 B)

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -2% - +2% (-1.39ms - +1.68ms)
    preact-local vs preact-main
  • 03_update10th1k_x16: unsure 🔍 -1% - +7% (-0.21ms - +3.05ms)
    preact-local vs preact-main
  • 07_create10k: unsure 🔍 -0% - +0% (-2.42ms - +3.86ms)
    preact-local vs preact-main
  • filter_list: unsure 🔍 -4% - +1% (-1.21ms - +0.15ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +1% (-2.37ms - +1.97ms)
    preact-local vs preact-main
  • many_updates: unsure 🔍 -9% - +2% (-1.70ms - +0.38ms)
    preact-local vs preact-main
  • text_update: unsure 🔍 -4% - +1% (-0.12ms - +0.02ms)
    preact-local vs preact-main
  • todo: faster ✔ 2% - 3% (1.00ms - 1.31ms)
    preact-local vs preact-main

usedJSHeapSize

  • 02_replace1k: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • 03_update10th1k_x16: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
    preact-local vs preact-main
  • 07_create10k: unsure 🔍 -0% - -0% (-0.00ms - -0.00ms)
    preact-local vs preact-main
  • filter_list: unsure 🔍 -2% - +0% (-0.02ms - +0.01ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main
  • many_updates: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main
  • text_update: unsure 🔍 -1% - +0% (-0.01ms - +0.00ms)
    preact-local vs preact-main
  • todo: faster ✔ 1% - 1% (0.01ms - 0.01ms)
    preact-local vs preact-main

Results

02_replace1k

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main82.59ms - 85.03ms-unsure 🔍
-2% - +2%
-1.68ms - +1.39ms		unsure 🔍
-3% - +1%
-2.42ms - +0.59ms
preact-local83.02ms - 84.89msunsure 🔍
-2% - +2%
-1.39ms - +1.68ms		-unsure 🔍
-2% - +1%
-2.05ms - +0.51ms
preact-hooks83.84ms - 85.61msunsure 🔍
-1% - +3%
-0.59ms - +2.42ms		unsure 🔍
-1% - +2%
-0.51ms - +2.05ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main3.32ms - 3.32ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms		faster ✔
1% - 1%
0.02ms - 0.02ms
preact-local3.32ms - 3.32msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-faster ✔
1% - 1%
0.02ms - 0.02ms
preact-hooks3.34ms - 3.34msslower ❌
1% - 1%
0.02ms - 0.02ms		slower ❌
1% - 1%
0.02ms - 0.02ms		-

run-warmup-0

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main33.11ms - 34.33ms-slower ❌
1% - 6%
0.39ms - 2.05ms		unsure 🔍
-1% - +4%
-0.20ms - +1.40ms
preact-local31.94ms - 33.06msfaster ✔
1% - 6%
0.39ms - 2.05ms		-unsure 🔍
-4% - +0%
-1.38ms - +0.15ms
preact-hooks32.60ms - 33.64msunsure 🔍
-4% - +1%
-1.40ms - +0.20ms		unsure 🔍
-0% - +4%
-0.15ms - +1.38ms		-

run-warmup-1

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main40.77ms - 42.84ms-unsure 🔍
-5% - +2%
-1.99ms - +0.84ms		unsure 🔍
-5% - +1%
-2.31ms - +0.48ms
preact-local41.41ms - 43.34msunsure 🔍
-2% - +5%
-0.84ms - +1.99ms		-unsure 🔍
-4% - +2%
-1.68ms - +1.00ms
preact-hooks41.78ms - 43.65msunsure 🔍
-1% - +6%
-0.48ms - +2.31ms		unsure 🔍
-2% - +4%
-1.00ms - +1.68ms		-

run-warmup-2

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main29.38ms - 30.51ms-unsure 🔍
-4% - +1%
-1.24ms - +0.27ms		faster ✔
0% - 5%
0.15ms - 1.51ms
preact-local29.93ms - 30.93msunsure 🔍
-1% - +4%
-0.27ms - +1.24ms		-unsure 🔍
-3% - +1%
-0.97ms - +0.28ms
preact-hooks30.40ms - 31.15msslower ❌
0% - 5%
0.15ms - 1.51ms		unsure 🔍
-1% - +3%
-0.28ms - +0.97ms		-

run-warmup-3

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main24.09ms - 24.93ms-unsure 🔍
-2% - +1%
-0.62ms - +0.31ms		unsure 🔍
-3% - +0%
-0.79ms - +0.11ms
preact-local24.47ms - 24.85msunsure 🔍
-1% - +3%
-0.31ms - +0.62ms		-unsure 🔍
-2% - +0%
-0.43ms - +0.06ms
preact-hooks24.69ms - 25.00msunsure 🔍
-0% - +3%
-0.11ms - +0.79ms		unsure 🔍
-0% - +2%
-0.06ms - +0.43ms		-

run-warmup-4

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main38.29ms - 40.46ms-unsure 🔍
-6% - +1%
-2.24ms - +0.38ms		faster ✔
3% - 9%
1.31ms - 3.77ms
preact-local39.57ms - 41.05msunsure 🔍
-1% - +6%
-0.38ms - +2.24ms		-faster ✔
2% - 6%
0.67ms - 2.54ms
preact-hooks41.35ms - 42.49msslower ❌
3% - 10%
1.31ms - 3.77ms		slower ❌
2% - 6%
0.67ms - 2.54ms		-

run-final

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main23.92ms - 25.03ms-unsure 🔍
-4% - +2%
-0.90ms - +0.51ms		unsure 🔍
-4% - +2%
-0.90ms - +0.38ms
preact-local24.24ms - 25.09msunsure 🔍
-2% - +4%
-0.51ms - +0.90ms		-unsure 🔍
-2% - +2%
-0.59ms - +0.46ms
preact-hooks24.43ms - 25.04msunsure 🔍
-2% - +4%
-0.38ms - +0.90ms		unsure 🔍
-2% - +2%
-0.46ms - +0.59ms		-
03_update10th1k_x16

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main43.43ms - 45.65ms-unsure 🔍
-7% - +0%
-3.05ms - +0.21ms		unsure 🔍
-5% - +2%
-2.38ms - +0.72ms
preact-local44.76ms - 47.16msunsure 🔍
-1% - +7%
-0.21ms - +3.05ms		-unsure 🔍
-2% - +5%
-1.02ms - +2.21ms
preact-hooks44.28ms - 46.45msunsure 🔍
-2% - +5%
-0.72ms - +2.38ms		unsure 🔍
-5% - +2%
-2.21ms - +1.02ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main3.27ms - 3.28ms-unsure 🔍
-0% - +0%
-0.01ms - +0.00ms		faster ✔
0% - 1%
0.02ms - 0.03ms
preact-local3.28ms - 3.29msunsure 🔍
-0% - +0%
-0.00ms - +0.01ms		-faster ✔
0% - 1%
0.01ms - 0.03ms
preact-hooks3.29ms - 3.31msslower ❌
0% - 1%
0.02ms - 0.03ms		slower ❌
0% - 1%
0.01ms - 0.03ms		-
07_create10k

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main929.31ms - 932.66ms-unsure 🔍
-0% - +0%
-3.86ms - +2.42ms		faster ✔
0% - 1%
1.30ms - 7.59ms
preact-local929.05ms - 934.36msunsure 🔍
-0% - +0%
-2.42ms - +3.86ms		-unsure 🔍
-1% - +0%
-7.48ms - +0.03ms
preact-hooks932.77ms - 938.09msslower ❌
0% - 1%
1.30ms - 7.59ms		unsure 🔍
-0% - +1%
-0.03ms - +7.48ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main26.33ms - 26.33ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms		unsure 🔍
-0% - -0%
-0.02ms - -0.02ms
preact-local26.33ms - 26.33msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-unsure 🔍
-0% - -0%
-0.02ms - -0.02ms
preact-hooks26.35ms - 26.35msunsure 🔍
+0% - +0%
+0.02ms - +0.02ms		unsure 🔍
+0% - +0%
+0.02ms - +0.02ms		-
filter_list

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main26.94ms - 28.16ms-unsure 🔍
-1% - +4%
-0.15ms - +1.21ms		unsure 🔍
-1% - +4%
-0.26ms - +1.07ms
preact-local26.72ms - 27.32msunsure 🔍
-4% - +1%
-1.21ms - +0.15ms		-unsure 🔍
-2% - +1%
-0.53ms - +0.28ms
preact-hooks26.88ms - 27.42msunsure 🔍
-4% - +1%
-1.07ms - +0.26ms		unsure 🔍
-1% - +2%
-0.28ms - +0.53ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main1.35ms - 1.37ms-unsure 🔍
-0% - +2%
-0.01ms - +0.02ms		unsure 🔍
-2% - +0%
-0.03ms - +0.00ms
preact-local1.34ms - 1.36msunsure 🔍
-2% - +0%
-0.02ms - +0.01ms		-faster ✔
1% - 3%
0.01ms - 0.04ms
preact-hooks1.37ms - 1.39msunsure 🔍
-0% - +3%
-0.00ms - +0.03ms		slower ❌
1% - 3%
0.01ms - 0.04ms		-
hydrate1k

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main142.87ms - 146.35ms-unsure 🔍
-1% - +2%
-1.97ms - +2.37ms		unsure 🔍
-1% - +2%
-1.35ms - +3.18ms
preact-local143.10ms - 145.71msunsure 🔍
-2% - +1%
-2.37ms - +1.97ms		-unsure 🔍
-1% - +2%
-1.24ms - +2.67ms
preact-hooks142.24ms - 145.15msunsure 🔍
-2% - +1%
-3.18ms - +1.35ms		unsure 🔍
-2% - +1%
-2.67ms - +1.24ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main6.10ms - 6.12ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms		faster ✔
0% - 1%
0.01ms - 0.04ms
preact-local6.10ms - 6.12msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms		-faster ✔
0% - 1%
0.01ms - 0.03ms
preact-hooks6.13ms - 6.14msslower ❌
0% - 1%
0.01ms - 0.04ms		slower ❌
0% - 1%
0.01ms - 0.03ms		-
many_updates

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main18.09ms - 19.80ms-unsure 🔍
-2% - +9%
-0.38ms - +1.70ms		unsure 🔍
-1% - +9%
-0.21ms - +1.66ms
preact-local17.70ms - 18.87msunsure 🔍
-9% - +2%
-1.70ms - +0.38ms		-unsure 🔍
-4% - +4%
-0.64ms - +0.76ms
preact-hooks17.84ms - 18.61msunsure 🔍
-9% - +1%
-1.66ms - +0.21ms		unsure 🔍
-4% - +3%
-0.76ms - +0.64ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main4.45ms - 4.46ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms		faster ✔
0% - 1%
0.00ms - 0.02ms
preact-local4.45ms - 4.46msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms		-faster ✔
0% - 1%
0.00ms - 0.02ms
preact-hooks4.46ms - 4.48msslower ❌
0% - 1%
0.00ms - 0.02ms		slower ❌
0% - 1%
0.00ms - 0.02ms		-
text_update

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main2.58ms - 2.68ms-unsure 🔍
-1% - +5%
-0.02ms - +0.12ms		faster ✔
5% - 10%
0.14ms - 0.27ms
preact-local2.53ms - 2.63msunsure 🔍
-4% - +1%
-0.12ms - +0.02ms		-faster ✔
7% - 11%
0.18ms - 0.32ms
preact-hooks2.79ms - 2.88msslower ❌
5% - 11%
0.14ms - 0.27ms		slower ❌
7% - 13%
0.18ms - 0.32ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main0.76ms - 0.77ms-unsure 🔍
-0% - +1%
-0.00ms - +0.01ms		faster ✔
3% - 4%
0.02ms - 0.03ms
preact-local0.76ms - 0.77msunsure 🔍
-1% - +0%
-0.01ms - +0.00ms		-faster ✔
4% - 4%
0.03ms - 0.04ms
preact-hooks0.79ms - 0.80msslower ❌
3% - 4%
0.02ms - 0.03ms		slower ❌
4% - 5%
0.03ms - 0.04ms		-
todo

duration

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main40.55ms - 40.75ms-slower ❌
3% - 3%
1.00ms - 1.31ms		unsure 🔍
-1% - +0%
-0.30ms - +0.12ms
preact-local39.38ms - 39.61msfaster ✔
2% - 3%
1.00ms - 1.31ms		-faster ✔
3% - 4%
1.03ms - 1.46ms
preact-hooks40.56ms - 40.92msunsure 🔍
-0% - +1%
-0.12ms - +0.30ms		slower ❌
3% - 4%
1.03ms - 1.46ms		-

usedJSHeapSize

VersionAvg timevs preact-mainvs preact-localvs preact-hooks
preact-main0.80ms - 0.80ms-slower ❌
1% - 1%
0.01ms - 0.01ms		faster ✔
2% - 2%
0.02ms - 0.02ms
preact-local0.80ms - 0.80msfaster ✔
1% - 1%
0.01ms - 0.01ms		-faster ✔
3% - 3%
0.02ms - 0.03ms
preact-hooks0.82ms - 0.82msslower ❌
2% - 2%
0.02ms - 0.02ms		slower ❌
3% - 3%
0.02ms - 0.03ms		-

tachometer-reporter-action v2 for Benchmarks

@coveralls
Copy link

coveralls commented Nov 7, 2023 •
edited
Loading

Coverage Status

coverage: 99.465% (-0.001%) from 99.466%
when pulling 6e534af on deopt-improvements
into f269b62 on main.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023 •
edited
Loading

Size Change: +5 B (0%)

Total Size: 59.7 kB

Filename Size Change
dist/preact.js 4.5 kB -1 B
dist/preact.min.js 4.53 kB +3 B (0%)
dist/preact.min.module.js 4.53 kB -1 B
dist/preact.min.umd.js 4.55 kB +1 B
dist/preact.module.js 4.52 kB -1 B
dist/preact.umd.js 4.56 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 4 kB 0 B
compat/dist/compat.module.js 3.93 kB 0 B
compat/dist/compat.umd.js 4.08 kB 0 B
debug/dist/debug.js 3.52 kB 0 B
debug/dist/debug.module.js 3.52 kB 0 B
debug/dist/debug.umd.js 3.6 kB 0 B
devtools/dist/devtools.js 232 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 315 B 0 B
hooks/dist/hooks.js 1.53 kB 0 B
hooks/dist/hooks.module.js 1.56 kB 0 B
hooks/dist/hooks.umd.js 1.62 kB 0 B
jsx-runtime/dist/jsxRuntime.js 963 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 938 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 1.04 kB 0 B
test-utils/dist/testUtils.js 453 B 0 B
test-utils/dist/testUtils.module.js 454 B 0 B
test-utils/dist/testUtils.umd.js 536 B 0 B

compressed-size-action

@andrewiggins andrewiggins changed the title Fix some deopts found in DeoptExplorer Do typeof string check before looking for String constructor Nov 8, 2023
@andrewiggins andrewiggins marked this pull request as ready for review November 8, 2023 02:10
marvinhagemeister
marvinhagemeister approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Great catch!

@andrewiggins andrewiggins merged commit b4a1cc2 into main Nov 8, 2023
12 checks passed
@andrewiggins andrewiggins deleted the deopt-improvements branch November 8, 2023 07:02
@JoviDeCroock JoviDeCroock mentioned this pull request Nov 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andrewiggins @coveralls @marvinhagemeister @JoviDeCroock