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

perf: Performance Improvements 1000% #95

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

medz
Copy link

@medz medz commented Dec 22, 2024

In the benchmark https://github.com/medz/dart-reactivity-benchmark?tab=readme-ov-file#score-ranking solidart performs very poorly

Because alien_signals is a scientific research library, it will not have subsequent bindings. Instead, it provides a Low-API for other packages to use.

So I used it to refactor solidart.

Current completion status:

  • Signal
  • Computed
  • Effect
  • Resource
  • batch
  • Signal List
  • Signal Set
  • Signal Map
  • Dev Tools - Completed part
  • Tests
  • flutter_solidart

Added

  • untrack

Other:
The core has been refactored. You need to implement it yourself to ensure compatibility with 1.x APIs.

Perf:
Benchmark improved by 1000% after refactoring

@medz
Copy link
Author

medz commented Dec 22, 2024

It's a pity that I can't continue to adapt more solidart ecosystem packages. Because there are really a lot of them, and I have more other work to do.

@medz
Copy link
Author

medz commented Dec 22, 2024

I referred to SolidJS APIs and Solidart APIs, and I tried to make it based on the original APIs of Solidart. There are some things I didn't implement because I don't understand the design. For example, dispose can be completely ignored in non-Flutter scenarios. Maybe I think adding dispose and binding it in flutter_solidart is a good choice. In addition, the prevValue function is very simple to implement, just add the prop of Signal. This is not common in Signals, on the contrary, computed still needs oldValue.

Summary: This is a draft PR, which is more of a reference. Because the alien_signals source code has no code comments, and it is implemented under strict restrictions, making the source code difficult to understand.
Therefore, this PR is very meaningful for reference.

You can consider continuing to improve solidart on the basis of PR❤️

@medz
Copy link
Author

medz commented Dec 22, 2024

In addition, the reason why I directly rely on the alien_signals library is more for the convenience of updating and maintenance.

If you think it is not good to rely on it, you can directly copy the system.dart file to solidart like vue core.

The reason why I rely on alien_signals is that it will constantly track the implementation, which can greatly avoid bugs in the system or more optimizations that are difficult to track and fix in time.

@johnsoncodehk
Copy link

If you think it is not good to rely on it, you can directly copy the system.dart file to solidart like vue core.

I really don't recommend this approach, now I can submit PR to alien-signals-dart to synchronize upstream updates when needed, but I can't afford to review correctness of code migration and monitor updates for more projects.

@medz
Copy link
Author

medz commented Dec 23, 2024

If you think it is not good to rely on it, you can directly copy the system.dart file to solidart like vue core.

I really don't recommend this approach, now I can submit PR to alien-signals-dart to synchronize upstream updates when needed, but I can't afford to review correctness of code migration and monitor updates for more projects.

Maybe you mean vue core?

@johnsoncodehk
Copy link

vue core is one of them, I will also monitor alien-signals-dart, alien-signals-lua, etc. to see if they port the code correctly, but I can't add more projects to my watchlist.

@nank1ro
Copy link
Owner

nank1ro commented Dec 30, 2024

I'm going to use this PR as a reference because it contains too many breaking changes. Will start the work again on another PR, thanks for your help @medz 💙

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.

3 participants