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

Update dependencies #275

Merged
merged 6 commits into from
Nov 25, 2020
Merged

Update dependencies #275

merged 6 commits into from
Nov 25, 2020

Conversation

Mephistophiles
Copy link
Contributor

No description provided.

Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
@osa1
Copy link
Owner

osa1 commented Nov 24, 2020

Thanks @Mephistophiles, this is really helpful.

I quickly compared number of dependencies and binary sizes, before and after this patch, for various build types:

Number of dependencies:

  • Default debug build:

    • master: 108
    • this PR: 121
    • diff: +12.03%
  • native-tls + desktop-notifications debug build:

    • master: 116
    • this PR: 128
    • diff: +10.34%
  • Default release build:

    • master: 109
    • this PR: 124
    • diff: +13.76%
  • native-tls + desktop-notifications release build:

    • master: 116
    • this PR: 130
    • diff: +12.06%

Binary sizes:

  • Default debug build

    • master: 56,363,696 bytes
    • this PR: 57,076,976
    • diff +1.26%
  • native-tls + desktop notifications debug build:

    • master: 51,954,168 bytes
    • this PR: 54,725,632 bytes
    • diff: +5.33%
  • Default release build:

    • master: 5,452,272
    • this PR: 5,436,304
    • diff: -0.29%
  • native-tls + desktop notifications release build:

    • master: 3,968,512
    • this PR: 3,960,792
    • diff: -0.19%

The summary is that this adds quite a lot of new dependencies, but interestingly final release binary sizes are actually almost the same (slightly smaller, <0.3%). I don't know if it's link-time optimizations, or single codegen unit, or what.

Some other notes:

  • We should probably disable unused features in time. This patch does it:

    diff
    diff --git a/libtiny_logger/Cargo.toml b/libtiny_logger/Cargo.toml
    index bcdb430..08e0e45 100644
    --- a/libtiny_logger/Cargo.toml
    +++ b/libtiny_logger/Cargo.toml
    @@ -9,4 +9,4 @@ edition = "2018"
     libtiny_common = { path = "../libtiny_common" }
     libtiny_ui = { path = "../libtiny_ui" }
     log = "0.4"
    -time = "0.2"
    +time = { version = "0.2", default-features = false, features = ["std"] }
    diff --git a/libtiny_tui/Cargo.toml b/libtiny_tui/Cargo.toml
    index d04eab3..fc39e98 100644
    --- a/libtiny_tui/Cargo.toml
    +++ b/libtiny_tui/Cargo.toml
    @@ -23,7 +23,7 @@ serde_yaml = "0.8.4"
     tempfile = "3.0.3"
     term_input = { path = "../term_input" }
     termbox_simple = { path = "../termbox" }
    -time = "0.2"
    +time = { version = "0.2", default-features = false }
     tokio = { version = "0.3.4", default-features = false, features = ["sync", "signal", "stream", "time"] }
     unicode-width = "0.1"
     
    diff --git a/libtiny_ui/Cargo.toml b/libtiny_ui/Cargo.toml
    index 847de88..0bd2382 100644
    --- a/libtiny_ui/Cargo.toml
    +++ b/libtiny_ui/Cargo.toml
    @@ -8,4 +8,4 @@ edition = "2018"
     [dependencies]
     libtiny_common = { path = "../libtiny_common" }
     dyn-clone = "1.0.3"
    -time = "0.2"
    +time = { version = "0.2", default-features = false }
    diff --git a/tiny/Cargo.toml b/tiny/Cargo.toml
    index a22c686..3911051 100644
    --- a/tiny/Cargo.toml
    +++ b/tiny/Cargo.toml
    @@ -28,7 +28,7 @@ libtiny_wire = { path = "../libtiny_wire" }
     log = "0.4"
     serde = { version = "1.0.8", features = ["derive"] }
     serde_yaml = "0.8.14"
    -time = "0.2"
    +time = { version = "0.2", default-features = false }
     tokio = { version = "0.3.4", default-features = false, features = [] }
     
     [build-dependencies]

    This saves us some binary size (though less than 0.1% as far as I can see)

  • dirs, env_logger, serde_yaml, and objekt bumps make no significant changes in the dep tree or binary sizes

  • With notify_rust bump we now have two copies of bitflags in Cargo.lock. Both versions seem to be used by the transitive deps of notify_rust.

  • With base64 bump we also duplicate it. The other version is used by rustls.

I should review the new time in more detail. The API changes quite a bit, and IIRC I had tried to bump it in the past and realized that there are some quite significant changes in Time struct layout, for example the new Time is much smaller than the old Tm and implements Copy.

In the meantime, it'd be helpful if you could disable unused features of time. You can use the diff I pasted above.

I'm OK with more dependencies because release builds don't seem to be affected.

@osa1
Copy link
Owner

osa1 commented Nov 25, 2020

It seems like this currently doesn't run at all. When I run tiny it crashes with this backtrace:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: IndeterminateOffset', libtiny_logger/src/lib.rs:124:14
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/std/src/panicking.rs:483
   1: core::panicking::panic_fmt
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/core/src/panicking.rs:85
   2: core::option::expect_none_failed
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/core/src/option.rs:1234
   3: libtiny_logger::print_header
   4: <libtiny_logger::Logger as libtiny_ui::UI>::new_server_tab
   5: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
   6: tiny::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@osa1 osa1 mentioned this pull request Nov 25, 2020
4 tasks
@osa1
Copy link
Owner

osa1 commented Nov 25, 2020

Debug mode backtrace:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: IndeterminateOffset', libtiny_logger/src/lib.rs:124:14
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/std/src/panicking.rs:483
   1: core::panicking::panic_fmt
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/core/src/panicking.rs:85
   2: core::option::expect_none_failed
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/core/src/option.rs:1234
   3: core::result::Result<T,E>::unwrap
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/core/src/result.rs:973
   4: libtiny_logger::print_header
             at ./libtiny_logger/src/lib.rs:123
   5: libtiny_logger::LoggerInner::new_server_tab
             at ./libtiny_logger/src/lib.rs:186
   6: <libtiny_logger::Logger as libtiny_ui::UI>::new_server_tab
             at ./libtiny_logger/src/lib.rs:41
   7: tiny::run::{{closure}}::{{closure}}
             at ./tiny/src/main.rs:127
   ...

@Mephistophiles
Copy link
Contributor Author

Mephistophiles commented Nov 25, 2020

Thank you for your review!

We should probably disable unused features in time. This patch does it:

done

With notify_rust bump we now have two copies of bitflags in Cargo.lock. Both versions seem to be used by the transitive deps of notify_rust.

removed from patchset

With base64 bump we also duplicate it. The other version is used by rustls.

removed from patchset

I should review the new time in more detail.

I will also take a closer look at this crate.

It seems like this currently doesn't run at all. When I run tiny it crashes with this backtrace:

I will switch this PR to draft mode before I fix it.

@Mephistophiles Mephistophiles changed the title Update dependencies [Draft] Update dependencies Nov 25, 2020
@Mephistophiles
Copy link
Contributor Author

I have collected binary size stats on my current commits:
dataset:

"nth","commit","debug.size","release.size"
1,"clear master",56368032,6030648
2,"tiny: bump dirs version 1.0.2 -> 3.0.1",56384088,6032632
3,"tiny: bump env_logger version 0.7.1 -> 0.8.2",56385888,6032832
4,"serde_yaml: bump version 0.7.1 -> 0.8.14",56215128,5996200
5,"time: dump version 0.1.44 -> 0.2.23",56956616,6016328
6,"libtiny_ui: bump objekt 0.1.2 -> 1.0.3 (crate was renamed to dyn-clone)",56945800,6014200
7,"libtiny_tui: bump tempfile version 3.0.3 -> 3.1.0",56945800,6014200
8,"cargo: update deps",56932224,6012144

debug size:
debug

release size:
release

@Mephistophiles
Copy link
Contributor Author

Using the new version of the time crate is not possible due to time-rs/time@01513cb.

Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
@osa1
Copy link
Owner

osa1 commented Nov 25, 2020

Using the new version of the time crate is not possible due to time-rs/time@01513cb.

Hmm, I think that issue exists in time-0.1 as well. We currently use this function to get current time: https://github.com/time-rs/time/blob/4235bd860d6c0fe074b06e172010d34e3399ffd8/src/lib.rs#L428-L430 which calls at in the same file, which calls sys::time_to_local_tm, which calls localtime_r: https://github.com/time-rs/time/blob/4235bd860d6c0fe074b06e172010d34e3399ffd8/src/sys.rs#L375

@osa1
Copy link
Owner

osa1 commented Nov 25, 2020

@Mephistophiles I suggest we remove the time bump from the commits for now.

@Mephistophiles Mephistophiles changed the title [Draft] Update dependencies Update dependencies Nov 25, 2020
@Mephistophiles
Copy link
Contributor Author

@osa1 ok, done.

@osa1
Copy link
Owner

osa1 commented Nov 25, 2020

Thanks. It seems like each commit builds on its own, so I'm merging without a merge commit.

@osa1 osa1 merged commit 997f6a8 into osa1:master Nov 25, 2020
@osa1
Copy link
Owner

osa1 commented Nov 25, 2020

If anyone's interested (maybe @Mephistophiles ) there's a bit of discussion in time's issue tracker on restoring the lost functionality in time-0.2: time-rs/time#293 (comment)

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.

2 participants