mullvad / windows-service-rs

Windows services in Rust
Apache License 2.0
525 stars 85 forks source link

Draft: move SCM-related functionality behind a feature flag #99

Closed citreae535 closed 1 year ago

citreae535 commented 1 year ago

This draft PR tries to move SCM-related functionality behind a feature flag with as few breaking changes as possible. In its current state I think there are no breaking changes.

The commit history is a bit complicated because I tried to preserve as much blame history as possible.

Current change details

Public changes:

  1. A new public module scm is added. This module is enabled by a new feature flag with the same name.
  2. service::Service and service_manager::ServiceManager are moved inside scm.
  3. SCM-related structs and enums are moved from service to private module scm::data. Public types are re-exported in scm.
  4. SCM-related bitflag enums are moved from service and service_manager to private module scm::flags. They are re-exported in scm.
  5. Three windows-sys features are moved behind the feature flag scm because they are only used inside module scm.

Private changes:

  1. SCM-related private helper functions inside service are moved to scm::utils.
  2. The private struct sc_handle::ScHandle is renamed to crate::scm::RawServiceHandle. The private module sc_handle is removed.
  3. The private module shell_scape and double_nul_terminated are moved inside scm.
  4. Many pub(crate) visibility modifiers are either changed to pub(super) or removed.
  5. ServiceState::from_raw, ServiceStatus::from_raw, and ServiceStatus::from_raw_ex are put behind the feature flag scm because they are only used inside module scm. The two structs themselves are kept in place because they are used when implementing Windows services.

To minimize breaking changes:

Unresolved questions

Side notes:


This change is Reviewable

faern commented 1 year ago

What's the motivation behind this pretty big restructure? Does it actually cut down the compile times in any substantial way? Are the resulting binaries smaller (even with optimizations on?)

faern commented 1 year ago

As a very general feedback point to this PR, without having looked at the content: scm is not a very user friendly abbreviation. I think I'd like it being spelled out as service-control-manager. Or potentially just control-manager if we think service is implied. But I don't like arcane abbreviations.

pronebird commented 1 year ago

A lot of restructuring, data.rs, flags.rs, utils.rs are very generic names too. It begs the question, does it all really worth it?

citreae535 commented 1 year ago

On performance, I tested building the crate itself and the binary crate shawl which only uses service implementation APIs and depends on windows-sys through windows-service. Each was repeated 3 times. The build command is cargo build --release --timings.

Mode Total windows-sys windows-service library/binary size
lib feature on 4s 2.6s 1.1s 1274 KB
lib feature off 3s 2.1s 0.6s 607 KB
bin feature on 25.99s 7.0s 4.0s 2391 KB
bin feature off 25.76s 5.3s 1.5s 2391 KB
faern commented 1 year ago

Disabling the feature flag when compiling the library itself cuts crate compile time by 50%

50% out of 1.1 seconds. So basically not any difference. Zero divided by two is still zero. To me that sounds like a bit too much churn for a bit too little gain.

and library size by 50%

But the binary size are the same. I guess the unused code gets shed at link time? A 50% library size difference I do think it's a great improvement! But if that size is always going to be shed off at link time anyway it's not going to make a difference in practice for anyone. Is the library in this case a shared .dll or a regular Rust rlib?

citreae535 commented 1 year ago

Well, Rust dylib is not easy to use, and this crate is already a wrapper to the Windows system dlls, so I don't think the size matters. I agree that this is too much effort for too little gain. Closing it now. Thanks a lot!