Compat: Add basic shims for some scheduler functions #2912

marvinhagemeister · 2021-01-05T21:04:48Z

Was notified of a library that makes use of some experimental concurrent mode APIs from React. This PR adds the necessary shims for that.

See https://github.com/dai-shi/use-context-selector/pull/36/files

cc @mischnic

github-actions · 2021-01-05T21:05:10Z

📊 Tachometer Benchmark Results

Summary

duration

02_replace1k: unsure 🔍 -1% - +1% (-0.83ms - +0.87ms)
^{preact-local vs preact-master}
03_update10th1k_x16: unsure 🔍 -4% - +4% (-1.32ms - +1.28ms)
^{preact-local vs preact-master}
07_create10k: unsure 🔍 -1% - +1% (-13.51ms - +17.84ms)
^{preact-local vs preact-master}
filter_list: unsure 🔍 -1% - +2% (-0.21ms - +0.53ms)
^{preact-local vs preact-master}
hydrate1k: unsure 🔍 -2% - +4% (-2.31ms - +5.64ms)
^{preact-local vs preact-master}
many_updates: faster ✔ 2% - 15% (0.83ms - 7.01ms)
^{preact-local vs preact-master}
text_update: unsure 🔍 -3% - +2% (-0.09ms - +0.06ms)
^{preact-local vs preact-master}

usedJSHeapSize

02_replace1k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
^{preact-local vs preact-master}
03_update10th1k_x16: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
^{preact-local vs preact-master}
07_create10k: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
^{preact-local vs preact-master}
filter_list: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
^{preact-local vs preact-master}
hydrate1k: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
^{preact-local vs preact-master}
many_updates: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
^{preact-local vs preact-master}
text_update: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
^{preact-local vs preact-master}

Results

02_replace1k

Browser: chrome-headless 87.0.4280.141
Sample size: 80
Built by: CI #707
Commit: 80a3cfd

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	159.43ms - 160.55ms	-	unsure 🔍 -1% - +1% -0.87ms - +0.83ms
preact-local	159.37ms - 160.65ms	unsure 🔍 -1% - +1% -0.83ms - +0.87ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	3.60ms - 3.61ms	-	unsure 🔍 -0% - +0% -0.01ms - +0.01ms
preact-local	3.60ms - 3.61ms	unsure 🔍 -0% - +0% -0.01ms - +0.01ms	-

run-warmup-0

Version	Avg time	vs preact-master	vs preact-local
preact-master	57.80ms - 58.44ms	-	unsure 🔍 -1% - +1% -0.56ms - +0.45ms
preact-local	57.78ms - 58.56ms	unsure 🔍 -1% - +1% -0.45ms - +0.56ms	-

run-warmup-1

Version	Avg time	vs preact-master	vs preact-local
preact-master	91.19ms - 92.03ms	-	unsure 🔍 -0% - +1% -0.01ms - +1.13ms
preact-local	90.67ms - 91.43ms	unsure 🔍 -1% - +0% -1.13ms - +0.01ms	-

run-warmup-2

Version	Avg time	vs preact-master	vs preact-local
preact-master	104.30ms - 105.39ms	-	unsure 🔍 -1% - +1% -0.89ms - +0.63ms
preact-local	104.45ms - 105.51ms	unsure 🔍 -1% - +1% -0.63ms - +0.89ms	-

run-warmup-3

Version	Avg time	vs preact-master	vs preact-local
preact-master	58.88ms - 59.39ms	-	unsure 🔍 -1% - +0% -0.47ms - +0.29ms
preact-local	58.94ms - 59.51ms	unsure 🔍 -0% - +1% -0.29ms - +0.47ms	-

run-warmup-4

Version	Avg time	vs preact-master	vs preact-local
preact-master	106.50ms - 111.53ms	-	unsure 🔍 -3% - +4% -3.21ms - +4.21ms
preact-local	105.78ms - 111.25ms	unsure 🔍 -4% - +3% -4.21ms - +3.21ms	-

run-final

Version	Avg time	vs preact-master	vs preact-local
preact-master	57.78ms - 58.25ms	-	unsure 🔍 -1% - +0% -0.50ms - +0.24ms
preact-local	57.86ms - 58.43ms	unsure 🔍 -0% - +1% -0.24ms - +0.50ms	-

03_update10th1k_x16

Browser: chrome-headless 87.0.4280.141
Sample size: 100
Built by: CI #707
Commit: 80a3cfd

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	30.16ms - 31.93ms	-	unsure 🔍 -4% - +4% -1.28ms - +1.32ms
preact-local	30.08ms - 31.98ms	unsure 🔍 -4% - +4% -1.32ms - +1.28ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	3.52ms - 3.53ms	-	unsure 🔍 -0% - +0% -0.01ms - +0.01ms
preact-local	3.52ms - 3.53ms	unsure 🔍 -0% - +0% -0.01ms - +0.01ms	-

07_create10k

Browser: chrome-headless 87.0.4280.141
Sample size: 50
Built by: CI #707
Commit: 80a3cfd

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	1478.48ms - 1498.14ms	-	unsure 🔍 -1% - +1% -17.84ms - +13.51ms
preact-local	1478.26ms - 1502.69ms	unsure 🔍 -1% - +1% -13.51ms - +17.84ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	25.99ms - 25.99ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.00ms
preact-local	25.99ms - 25.99ms	unsure 🔍 -0% - +0% -0.00ms - +0.00ms	-

filter_list

Browser: chrome-headless 87.0.4280.141
Sample size: 50
Built by: CI #707
Commit: 80a3cfd

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	23.96ms - 24.44ms	-	unsure 🔍 -2% - +1% -0.53ms - +0.21ms
preact-local	24.08ms - 24.65ms	unsure 🔍 -1% - +2% -0.21ms - +0.53ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	1.60ms - 1.60ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.00ms
preact-local	1.60ms - 1.60ms	unsure 🔍 -0% - +0% -0.00ms - +0.00ms	-

hydrate1k

Browser: chrome-headless 87.0.4280.141
Sample size: 50
Built by: CI #707
Commit: 80a3cfd

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	151.83ms - 157.81ms	-	unsure 🔍 -4% - +1% -5.64ms - +2.31ms
preact-local	153.87ms - 159.10ms	unsure 🔍 -2% - +4% -2.31ms - +5.64ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	6.19ms - 6.19ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.00ms
preact-local	6.19ms - 6.19ms	unsure 🔍 -0% - +0% -0.00ms - +0.00ms	-

many_updates

Browser: chrome-headless 87.0.4280.141
Sample size: 70
Built by: CI #707
Commit: 80a3cfd

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	42.99ms - 47.50ms	-	slower ❌ 2% - 17% 0.83ms - 7.01ms
preact-local	39.21ms - 43.44ms	faster ✔ 2% - 15% 0.83ms - 7.01ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	4.85ms - 4.85ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.00ms
preact-local	4.85ms - 4.85ms	unsure 🔍 -0% - +0% -0.00ms - +0.00ms	-

text_update

Browser: chrome-headless 87.0.4280.141
Sample size: 50
Built by: CI #707
Commit: 80a3cfd

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	2.81ms - 2.92ms	-	unsure 🔍 -2% - +3% -0.06ms - +0.09ms
preact-local	2.79ms - 2.89ms	unsure 🔍 -3% - +2% -0.09ms - +0.06ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	0.83ms - 0.83ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.00ms
preact-local	0.83ms - 0.83ms	unsure 🔍 -0% - +0% -0.00ms - +0.00ms	-

_{tachometer-reporter-action v2 for CI}

github-actions · 2021-01-05T21:07:12Z

Size Change: +371 B (0%)

Total Size: 42.3 kB

Filename	Size	Change
`compat/dist/compat.js`	3.45 kB	+114 B (3%)
`compat/dist/compat.module.js`	3.47 kB	+140 B (4%)
`compat/dist/compat.umd.js`	3.51 kB	+117 B (3%)

ℹ️ View Unchanged

Filename	Size	Change
`debug/dist/debug.js`	2.99 kB	0 B
`debug/dist/debug.module.js`	2.98 kB	0 B
`debug/dist/debug.umd.js`	3.07 kB	0 B
`devtools/dist/devtools.js`	185 B	0 B
`devtools/dist/devtools.module.js`	195 B	0 B
`devtools/dist/devtools.umd.js`	261 B	0 B
`dist/preact.js`	4.04 kB	0 B
`dist/preact.min.js`	4.07 kB	0 B
`dist/preact.module.js`	4.06 kB	0 B
`dist/preact.umd.js`	4.11 kB	0 B
`hooks/dist/hooks.js`	1.13 kB	0 B
`hooks/dist/hooks.module.js`	1.15 kB	0 B
`hooks/dist/hooks.umd.js`	1.2 kB	0 B
`jsx-runtime/dist/jsxRuntime.js`	327 B	0 B
`jsx-runtime/dist/jsxRuntime.module.js`	335 B	0 B
`jsx-runtime/dist/jsxRuntime.umd.js`	405 B	0 B
`test-utils/dist/testUtils.js`	437 B	0 B
`test-utils/dist/testUtils.module.js`	439 B	0 B
`test-utils/dist/testUtils.umd.js`	515 B	0 B

_{compressed-size-action}

coveralls · 2021-01-05T21:07:27Z

Coverage decreased (-0.2%) to 99.435% when pulling 5d24695 on compat-cm-shims into 989315c on master.

mischnic · 2021-01-05T21:09:06Z

Great! There's also at least one export on react-dom: https://github.com/dai-shi/use-context-selector/blob/master/src/batchedUpdates.ts

marvinhagemeister · 2021-01-05T23:00:11Z

@mischnic That's already present in preact/compat :)

preact/compat/src/index.js

Line 101 in 989315c

const unstable_batchedUpdates = (callback, arg) => callback(arg);

mischnic · 2021-01-05T23:13:20Z

🙈

yisar · 2021-01-14T03:18:12Z

I don't know why preact must insist on running.
In fact, this kind of implementation is totally incorrect. The update model of preact is completely different from react, which will lead to more and more differences.

marvinhagemeister · 2021-01-14T08:49:44Z

@yisar I'm not sure if you're aware, but our implementation of __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED isn't the same as in React either. And yet it keeps working like a charm. The surprising thing of working on the compat layer is that actual libraries are way less reliant on specific implementation details than one might assume. Like the library linked in the original comment. It doesn't rely on specific behavior, but rather just calls these functions and that's it.

I don't know why preact must insist on running.

Going into other frameworks issue trackers and posting something akin to "your framework sucks" is just rude. You might want to take a look at our Code of Conduct page again.

yisar · 2021-01-14T09:01:04Z

@marvinhagemeister Once you introduce the shim, a lot of disagreements will arise because the shim is inaccurate.

In essence, this API is not just to execute a callback. The reason why use-context-selector uses this API is to solve a tearing problem, which does not exist in preact.

Of course, this is not important. What I need to explain is that once the shim is introduced, there will be more differences. The introduction of shim is not a good behavior, it is full of uncertainty and instability.

It's also worth mentioning that I don't think preact is terrible, I just think it's unwise to introduce an unnecessary shim.

You can give up use-context-selector, can't you?

cristianbote · 2021-01-14T09:19:14Z

@yisar preact does not need to have accurate scheduler implementation, as you noted as well, so why not have the APIs from scheduler exported for the libraries that make use of them? Regardless of their accuracy, as long as they do not break the functionality, there's no harm in having them.

Marvin already added a block of comment where all of the concerns that one might have, are addressed. So, any discussion regarding the accuracy, or validity of them should start from there.

cristianbote

Great job @marvinhagemeister! This will make the adoption of libraries much easier 🎉

yisar · 2021-01-14T09:36:59Z

@cristianbote

so why not have the APIs from scheduler exported for the libraries that make use of them

You're right. If it's just for sharing the third-party library, it can be done without causing damage.

But what if users need to implement their own libraries with the APIs of the preact/compat ? At this point, these shims will bring a lot of trouble, because these APIs should not be here. Their semantics and uses should not be here.

I notice that this PR has been merged, and I don't want to argue about this.

Compat: Add basic shims for some scheduler functions

5d24695

Merge branch 'master' into compat-cm-shims

6dcb02a

cristianbote approved these changes Jan 14, 2021

View reviewed changes

marvinhagemeister merged commit ee76c1e into master Jan 14, 2021

marvinhagemeister deleted the compat-cm-shims branch January 14, 2021 09:27

Compat: Add basic shims for some scheduler functions #2912

Compat: Add basic shims for some scheduler functions #2912

Conversation

marvinhagemeister commented Jan 5, 2021

github-actions bot commented Jan 5, 2021 • edited Loading

📊 Tachometer Benchmark Results

Summary

duration

usedJSHeapSize

Results

duration

usedJSHeapSize

run-warmup-0

run-warmup-1

run-warmup-2

run-warmup-3

run-warmup-4

run-final

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

github-actions bot commented Jan 5, 2021 • edited Loading

coveralls commented Jan 5, 2021

mischnic commented Jan 5, 2021

marvinhagemeister commented Jan 5, 2021

mischnic commented Jan 5, 2021

yisar commented Jan 14, 2021

marvinhagemeister commented Jan 14, 2021

yisar commented Jan 14, 2021

cristianbote commented Jan 14, 2021

cristianbote left a comment

Choose a reason for hiding this comment

yisar commented Jan 14, 2021 • edited Loading

github-actions bot commented Jan 5, 2021 •

edited

Loading

github-actions bot commented Jan 5, 2021 •

edited

Loading

yisar commented Jan 14, 2021 •

edited

Loading