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

Improve sass --embedded performance #2013

Merged
merged 15 commits into from
Sep 1, 2023
Merged

Improve sass --embedded performance #2013

merged 15 commits into from
Sep 1, 2023

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Jun 11, 2023

This PR improves the performance by doing the following changes.

  • The wire packets of Record type (int, Uint8List) are decoded as streaming.
  • Each isolate channel and its dispatcher is persisted rather than recreated for each compilation.
  • The communication between main isolate uses Record type (bool, Uint8List), therefore allow a boolean signal to be passed to deactivate the channel at the correct timing, to fix the race condition.
  • Use Mailbox via ffi to provide synchronized communication between isolates.

Performance summary:

  • For lots of tiny files
    • Single-threaded performance has been improved a lot and getting closer to old single isolate performance.
    • Multi-threaded performance beats all previous versions and even beats multi-processes.
  • For high number of round trips (function calls, importers, etc)
    • It beats all previous versions in all running modes (single-threaded, multi-threaded or multi-processes).
  • For large files like bootstrap
    • Almost no performance difference in all running modes as this test is CPU & I/O bounded. Multi-processes mode is fastest for any version because multi-threaded mode is bottlenecked on I/O.

@ntkme ntkme changed the title Prase packets on transformation and persist isolate channels Improve sass --embedded performance Jun 11, 2023
@jathak jathak requested a review from nex3 June 13, 2023 20:44
@nex3
Copy link
Contributor

nex3 commented Jun 13, 2023

I would like to avoid using FFI for this until there's a Dart-Team-owned package available for it. I don't feel confident in my ability to own and maintain that code.

@ntkme
Copy link
Contributor Author

ntkme commented Jun 13, 2023

@mraleph I took your Mailbox implementation and made it work with the current dart-sass. However, we would like to see if Dart Team can own the FFI part and ship it as a package that we can consume. Would you or someone from Dart SDK team be able to help us? Thanks in advance!

Reference: dart-lang/sdk#52121

@mraleph
Copy link

mraleph commented Jun 15, 2023

@ntkme yes, we are going to provide the package as agreed. I was planning to take a look at this in the next month.

@ntkme
Copy link
Contributor Author

ntkme commented Jun 16, 2023

@nex3 I will split this up into smaller PRs.

@ntkme ntkme marked this pull request as draft June 16, 2023 04:08
@nex3 nex3 force-pushed the main branch 2 times, most recently from 9cff4ca to 6c59213 Compare July 21, 2023 01:57
@mraleph
Copy link

mraleph commented Aug 9, 2023

@ntkme we have published https://pub.dev/packages/native_synchronization

@ntkme ntkme marked this pull request as ready for review August 10, 2023 09:58
@ntkme
Copy link
Contributor Author

ntkme commented Aug 17, 2023

@nex3 Welcome back. This PR is now ready for review.

@nex3
Copy link
Contributor

nex3 commented Aug 17, 2023

Thanks for the ping. It may be a few days before I can get around to this, but it's on my radar.

lib/src/embedded/dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
@ntkme ntkme requested a review from nex3 August 25, 2023 03:21
ntkme and others added 2 commits August 25, 2023 14:40
This encapsulates the logic of repurposing an existing isolate
connection to minimize mutability. Instead, messages from the isolate
are exposed as a separate Stream for each compilation.
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
lib/src/embedded/isolate_dispatcher.dart Outdated Show resolved Hide resolved
@ntkme ntkme requested a review from nex3 August 30, 2023 01:14
@ntkme
Copy link
Contributor Author

ntkme commented Aug 30, 2023

@nex3 Huge thanks for refactoring the reuse logic and remove some left-overs. I've reverted the commit of bubbling up ProtocolError back to original implementation and updated documentation accordingly.

@nex3
Copy link
Contributor

nex3 commented Sep 1, 2023

Thanks!

@nex3 nex3 merged commit af0118a into sass:main Sep 1, 2023
45 checks passed
@ntkme ntkme deleted the refactor branch September 1, 2023 23:13
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.

None yet

3 participants