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

Incorrect timing #932

Closed
yisar opened this issue Aug 29, 2021 · 14 comments
Closed

Incorrect timing #932

yisar opened this issue Aug 29, 2021 · 14 comments

Comments

@yisar
Copy link
Contributor

yisar commented Aug 29, 2021

https://github.com/yisar/js-framework-benchmark/tree/master/frameworks/keyed/fre

When I run the benchmark of fre, the time of select rows exceeds 1s. There must be a problem here, because when I operate in chrome, select rows only takes 20ms.
My chrome:

微信图片_20210829104200

image

The benchmark:

image

There must be an error. It can't be so slow unless it's blocked.

In addition, fre is a framework with Time slicing. It seems that the current benchmark is not friendly, maybe.

frejs/fre#271 (comment)
#498 (comment)

Can you take a look?

@lxsmnsyc
Copy link
Contributor

20ms on normal run (with no CPU throttle) is slow (in comparison, Solid and compostate-jsx only takes <1ms). Most of the VDOM libraries fail on this test since it requires rebuilding the entire VDOM of the table rather than updating the selected row.

@yisar
Copy link
Contributor Author

yisar commented Aug 29, 2021

@lxsmnsyc You're right. In this case, the fine-grained update of solidjs with in-place mutation can be completed in 1ms, but I mean, whether it's 20ms or 1ms, humans can't perceive it, and chasing 1ms is of little significance.

Fre also has the defect of VDOM, that is, it needs to traverse the whole list, which is a problem of all VDOM frameworks, because the mental model of immediate mode is like this.

@lxsmnsyc
Copy link
Contributor

lxsmnsyc commented Aug 29, 2021

whether it's 20ms or 1ms, humans can't perceive it

Well the thing is, the benchmarks are for stress testing and not for normal user interaction. Almost every framework/libraries in the benchmarks have no noticeable differences in performance, well at least on human perception.

IMO it would better if you can compare the benchmarks against similar libraries (for instance, Preact and React).

Another thing is that the select row test has been in many discussions before. IIRC the final decision was to run the test on x16 throttling. Otherwise, if it is indeed slower than expected, then let's wait for resolution.

@ryansolid
Copy link
Contributor

@yisar are you running on M1 apple chips? They have an issue with applied slowdowns when running in the chrome driver. I need to turn off slowdown on my main development computer since it is so absurdly long. Do other implementations have the same issue on your computer?

@yisar
Copy link
Contributor Author

yisar commented Aug 29, 2021

@ryansolid No, other frameworks work well.
I'm not sure what I missed. It's also possible that the test program deliberately magnified fre's defects.
As long as the reason can be found, there is a way to optimize the test case.

@yisar
Copy link
Contributor Author

yisar commented Aug 29, 2021

IIRC the final decision was to run the test on x16 throttling.

It is possible to amplify fre's defects here because fre does not use memo technology.

@lxsmnsyc
Copy link
Contributor

I suggest that you look into the diff/patch implementation of fre

@ryansolid
Copy link
Contributor

Ok, well 20ms with no slowdown I think is still indicator of something. Can try to turn off slowdowns to confirm that. I comment out https://github.com/krausest/js-framework-benchmark/blob/master/webdriver-ts/src/benchmarks.ts#L92 and rebuild to run with no throttling.

@yisar
Copy link
Contributor Author

yisar commented Aug 29, 2021

@ryansolid You're right. It's really caused by slowdown. If so, I must use memo technology.

@yisar
Copy link
Contributor Author

yisar commented Aug 29, 2021

@ryansolid @lxsmnsyc

image

You're right. It's true that the slowdown magnifies fre's defects, but even if there is no slowdown, fre's update is still a bit slow... Of course, Time slicing itself will slow down as a whole, but at least the current result is expected.

@lxsmnsyc
Copy link
Contributor

It seems that the partial update is slower than React, which usually indicates that the patch algo is slower. If you can optimize that part, I'm sure that the rest of patch-related tests would perform well.

@yisar
Copy link
Contributor Author

yisar commented Aug 29, 2021

@lxsmnsyc Yes, yes, this part can be optimized. It needs to diff props in the reconcile stage. I didn't do this now 😭 :

@krausest
Copy link
Owner

I suggest you create a PR (we can still decide when to merge this PR) and then we can take a look at it together. The implementation on https://github.com/yisar/js-framework-benchmark/tree/master/frameworks/keyed/fre seems to have no remove button and thus my build process fails.

@yisar
Copy link
Contributor Author

yisar commented Aug 29, 2021

@krausest pr is here #933

@yisar yisar closed this as completed Aug 30, 2021
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

No branches or pull requests

4 participants