sdk: lower compile times #3879

bnjbvr · 2024-08-22T11:21:23Z

This series of patches, investigated using Fasterthanlime's good advice, helped lowering the compiles times from 44 seconds to 4 on my machine, for the matrix-sdk-ui crate.

Compile times for the matrix-sdk crate have also been lowered from 33 to 12 seconds.

Overall, for a plain cargo build:

before: 113 seconds
after: 54 seconds

Each commit can be reviewed independently.

Codecov Report

Attention: Patch coverage is 72.01835% with 61 lines in your changes missing coverage. Please review.

Project coverage is 84.03%. Comparing base (2db031c) to head (610c1b8).
Report is 15 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-ui/src/timeline/traits.rs	58.10%	31 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	73.33%	28 Missing ⚠️
...rates/matrix-sdk-ui/src/room_list_service/state.rs	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3879      +/-   ##
==========================================
- Coverage   84.10%   84.03%   -0.07%     
==========================================
  Files         266      266              
  Lines       27724    27731       +7     
==========================================
- Hits        23318    23305      -13     
- Misses       4406     4426      +20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr · 2024-08-22T15:32:47Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

+        if let EventTimelineItemKind::Remote(remote_event) = &item.kind {
+            if let Flow::Remote { encryption_info, .. } = &self.ctx.flow {
+                new_item = new_item.with_kind(EventTimelineItemKind::Remote(
+                    remote_event.with_encryption_info(encryption_info.clone()),
+                ));
+            }


A small note: this is slightly changing behavior, because I thought the previous behavior was buggy.

Previous behavior did this: if the item comes from a local echo, use an encryption_info set to None, and replace it in the original timeline item.

With the new behavior, we override the encryption_info only if the new information comes from a remote echo.

Incidentally, @pixlwave opened an issue which I think describes that, today: #3882. Happy to move the change to a separate PR if it's preferred; since it was only a 3 lines change, I deemed it not useful. But it's worth highlighting for the review here, since this is the only functional change of this PR overall.

Yep, I can confirm that this fixes the issue 👍

I'm fine keeping the change here.

…it with functions After this, compiles times for matrix-sdk-ui are reduced from 44 seconds to 30 seconds on my machine.

These are function parameters, they shouldn't be cloned for no good reason.

It was found that it's making the whole build slower because it's hitting a pathologically slow path in type-checking. Considering that it doesn't do much, let's get rid of it and inline it instead. After this, compiles times are reduced from 30 seconds to 22 seconds on my machine

It's one fewer pathologically slow type query during type-checking, and a build time for matrix-sdk-ui 5 seconds lower.

Who has two thumbs and wants to make a trait not use async_trait, and thus get rid of trait object safety?

This divides compile times for matrix-sdk by 2, on my machine (33 seconds -> 16).

Hywan

I love what I've read. Excellent work!

Hywan · 2024-08-23T07:27:25Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

@@ -103,7 +102,6 @@ pub(super) enum Flow {
    },
 }

-#[derive(Clone)]


Hywan · 2024-08-23T07:27:27Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

@@ -73,7 +73,6 @@ use crate::{
 };

 /// When adding an event, useful information related to the source of the event.
-#[derive(Clone)]


Hywan · 2024-08-23T07:29:37Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

+        if let EventTimelineItemKind::Remote(remote_event) = &item.kind {
+            if let Flow::Remote { encryption_info, .. } = &self.ctx.flow {
+                new_item = new_item.with_kind(EventTimelineItemKind::Remote(
+                    remote_event.with_encryption_info(encryption_info.clone()),
+                ));
+            }


I'm fine keeping the change here.

Hywan · 2024-08-23T07:30:30Z

crates/matrix-sdk-ui/src/timeline/traits.rs

@@ -43,7 +44,7 @@ pub trait RoomExt {
    /// independent events.
    ///
    /// This is the same as using `room.timeline_builder().build()`.
-    async fn timeline(&self) -> Result<Timeline, timeline::Error>;
+    fn timeline(&self) -> impl Future<Output = Result<Timeline, timeline::Error>> + Send;


I'm even wondering if we can use async fn in traits since we are targeting a recent Rust version. I'm lazy checking though. I'm fine with -> impl Future, which is exactly the same.

I was coming back here to ensure the + Send bound is present. It is present. Well done :-].

Thanks for the review!

The compiler warns if we use async fn, and we have some builds with warn-as-errors, so it fails those builds. The reasoning is that using an async fn in trait doesn't tell anything about the bounds of the future, so it eventually is it that recommended to return an impl Future.

Hywan · 2024-08-23T07:36:24Z

crates/matrix-sdk-ui/Cargo.toml

@@ -26,7 +26,6 @@ async_cell = "0.2.2"
 async-once-cell = "0.5.2"
 async-rx = { workspace = true }
 async-stream = { workspace = true }
-async-trait = { workspace = true }


bnjbvr requested a review from a team as a code owner August 22, 2024 11:21

bnjbvr requested review from jmartinesp and Hywan and removed request for a team and jmartinesp August 22, 2024 11:21

bnjbvr mentioned this pull request Aug 22, 2024

sdk: lower compile times too by using async_trait less #3880

Closed

bnjbvr force-pushed the bnjbvr/lower-compile-times-sdk-ui branch 2 times, most recently from 227aa93 to 692561f Compare August 22, 2024 14:15

bnjbvr changed the title ~~sdk-ui: lower compile times~~ sdk: lower compile times Aug 22, 2024

Hywan self-assigned this Aug 22, 2024

bnjbvr commented Aug 22, 2024

View reviewed changes

bnjbvr added 10 commits August 22, 2024 17:51

room list service: get rid of the abstract action system and replace …

4ea2078

…it with functions After this, compiles times for matrix-sdk-ui are reduced from 44 seconds to 30 seconds on my machine.

timeline: get rid of Clone for Flow and TimelineEventContext

303855f

These are function parameters, they shouldn't be cloned for no good reason.

timeline: don't use async_trait for RoomExt

23ea6dc

It's one fewer pathologically slow type query during type-checking, and a build time for matrix-sdk-ui 5 seconds lower.

pinned events: get rid of async_trait for PinnedEventsRoom too

77b706c

timeline: get rid of async_trait for our own traits

ff7342d

paginator: use a generic type instead of a boxed trait

da363e5

Who has two thumbs and wants to make a trait not use async_trait, and thus get rid of trait object safety?

paginator: don't use async_trait for PaginableRoom

c582320

This divides compile times for matrix-sdk by 2, on my machine (33 seconds -> 16).

room edit: don't use async_trait for EventSource either

fde3e29

sdk-ui: remove dependency to async_trait \o/

610c1b8

bnjbvr force-pushed the bnjbvr/lower-compile-times-sdk-ui branch from 692561f to 610c1b8 Compare August 22, 2024 15:59

Hywan approved these changes Aug 23, 2024

View reviewed changes

bnjbvr merged commit be13388 into main Aug 23, 2024
40 checks passed

bnjbvr deleted the bnjbvr/lower-compile-times-sdk-ui branch August 23, 2024 07:43

jmartinesp mentioned this pull request Aug 23, 2024

feat(sdk): Live updates for pinned events timeline with replacements and redactions #3840

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: lower compile times #3879

sdk: lower compile times #3879

bnjbvr commented Aug 22, 2024 •

edited

Loading

poljar commented Aug 22, 2024

codecov bot commented Aug 22, 2024 •

edited

Loading

bnjbvr Aug 22, 2024 •

edited

Loading

pixlwave Aug 22, 2024

Hywan Aug 23, 2024

Hywan left a comment

Hywan Aug 23, 2024

Hywan Aug 23, 2024

Hywan Aug 23, 2024

Hywan Aug 23, 2024

Hywan Aug 23, 2024

bnjbvr Aug 23, 2024 •

edited

Loading

Hywan Aug 23, 2024

sdk: lower compile times #3879

sdk: lower compile times #3879

Conversation

bnjbvr commented Aug 22, 2024 • edited Loading

poljar commented Aug 22, 2024

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

bnjbvr Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

bnjbvr Aug 22, 2024 •

edited

Loading

bnjbvr Aug 23, 2024 •

edited

Loading