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

optimize compat by hoisting the vnode handling func #3941

Merged
merged 4 commits into from
Mar 22, 2023

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Mar 20, 2023

This seems to significantly improve performance from a V8 Point of View for RTS

Text:
compat old x 240.57 ops/sec ±1.82 (81 runs sampled)
compat new x 299.133 ops/sec ±3.07 (82 runs sampled)

Fastest is compat new

SearchResults:
compat old x 630.577 ops/sec ±3.12 (83 runs sampled)
compat new x 1,040.508 ops/sec ±3.96 (82 runs sampled)

Fastest is compat new

ColorPicker:
compat old x 2,867.721 ops/sec ±3.26 (88 runs sampled)
compat new x 6,103.969 ops/sec ±4.12 (85 runs sampled)

Fastest is compat new

current is this, compat is the 10.13.1 release

@github-actions
Copy link

github-actions bot commented Mar 20, 2023

Size Change: +55 B (0%)

Total Size: 54.3 kB

Filename Size Change
compat/dist/compat.js 3.91 kB +21 B (0%)
compat/dist/compat.module.js 3.84 kB +16 B (0%)
compat/dist/compat.umd.js 3.98 kB +18 B (0%)
ℹ️ View Unchanged
Filename Size Change
debug/dist/debug.js 3 kB 0 B
debug/dist/debug.module.js 3.01 kB 0 B
debug/dist/debug.umd.js 3.08 kB 0 B
devtools/dist/devtools.js 231 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 315 B 0 B
dist/preact.js 4.21 kB 0 B
dist/preact.min.js 4.24 kB 0 B
dist/preact.min.module.js 4.24 kB 0 B
dist/preact.min.umd.js 4.27 kB 0 B
dist/preact.module.js 4.23 kB 0 B
dist/preact.umd.js 4.28 kB 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.61 kB 0 B
jsx-runtime/dist/jsxRuntime.js 360 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 326 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 441 B 0 B
test-utils/dist/testUtils.js 442 B 0 B
test-utils/dist/testUtils.module.js 444 B 0 B
test-utils/dist/testUtils.umd.js 526 B 0 B

compressed-size-action

@coveralls
Copy link

coveralls commented Mar 20, 2023

Coverage Status

Coverage: 99.547% (-0.0007%) from 99.548% when pulling 9658dcf on optimize-compat-vnode into 958311a on master.

@JoviDeCroock JoviDeCroock force-pushed the optimize-compat-vnode branch from 8b42294 to d9abd68 Compare March 20, 2023 08:38

if (props.class != props.className) {
Copy link
Member Author

Choose a reason for hiding this comment

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

What happened before:

  • props.class === undefined && props.className !== undefined --> we would make className enumerable and make class === className
  • props.class !== undefined && props.className === undefined --> we would make className not enumerable and make className return props.class
  • props.class !== undefined && props.className !== undefined --> we would make className enumerable and make class === className

Position 1 and 3 are different because we are just returning the value for className rather than the one for class, I don't think this matters.
Position 2 is different because we aren't introducing a getter for className to return class, I don't think this matters

CC @marvinhagemeister

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking. That sounds reasonable. I'm good with merging the PR 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yep that seems reasonable to me. I think the dynamic assignment of enumerable here was from when we used to make class a descriptor too.

@JoviDeCroock JoviDeCroock merged commit 6adfd34 into master Mar 22, 2023
@JoviDeCroock JoviDeCroock deleted the optimize-compat-vnode branch March 22, 2023 12:57
JoviDeCroock added a commit that referenced this pull request Jan 16, 2024
JoviDeCroock added a commit that referenced this pull request Jan 16, 2024
* backport #3911

* backport #3906

* backport #3837

* backport #3908

* backport #3904

* backport #3905

* backport #3898

* backport #3910

* backport #3948

* backport #3941

* backport #3945

* backport #3919

* backport #3922

* backport #3921

* backport #3903

* fix lint

* update more

* debug
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.

5 participants