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

Add direct_target to Room struct #104

Merged
merged 7 commits into from
Oct 8, 2020
Merged

Conversation

martinetd
Copy link
Contributor

Add an optional direct_target field to Room struct to track rooms marked as "direct" : for direct rooms, it will contain the associated UserId.

Closes #103.

Notes:

  • Unfortunately was a bit more work than I had expected because we only used to parse account events tied to rooms, not the global ones -- in particular I'm not sure how to handle the saving updated rooms for the general event, the usual "return bool" doesn't work well the helper should return a vect of updated rooms or something it's a bit annoying. Probably want to fix this.
  • I exposed the RoomMember struct more visibly so users wouldn't need to explicitely declare the matrix_sdk_base crate -- not sure if we want that but it's been handy for my testing from outside
  • ... speaking of which, probably want to add some test. I've seen how hand-written json samples are used at the end just not sure if we want a new test or if it could integrate in one?

@martinetd
Copy link
Contributor Author

Tests failing due to type length limit; I'm not comfortable changing that in a lib (and wouldn't know where to put it anyway).

I'd say it was a preexisting problem that would deserve its own PR even if that means delaying this.

Reading up on it, it looks like "boxing" things helps split types down?
https://users.rust-lang.org/t/exponential-type-size-of-async-function/36852
But I don't think I understand most of what I'm reading about it right now, or at least how it applies to this project -- having a way to measure individual types / find where the "big types" are would help perhaps...
OTOH I like what I'm reading, I was thinking the compilation was very slow but if we improve the future chains type size it'll apparently speed up considerably, that's nice! :D

@poljar
Copy link
Contributor

poljar commented Oct 8, 2020

Tests failing due to type length limit; I'm not comfortable changing that in a lib (and wouldn't know where to put it anyway).

I think we can just bump the limits, there are a couple of issues in rustc with this, relevant issue is over here rust-lang/rust#54540. At the bottom mostly, there was a regression as it seems. A new Rust release is scheduled quite soon so we'll be able to remove them once this is fixed.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, couple of grammar issues.

As I mentioned already all of this will change in the future once we get rid of our snapshot based state store.

@@ -695,6 +695,25 @@ impl BaseClient {
// }
}

/// Handle a m.direc event, updating room states if necessary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Handle a m.direc event, updating room states if necessary.
/// Handle a m.direc event, updating the room states if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This event is not tied to a room but multiple rooms, made rooms plural instead.)

matrix_sdk_base/src/models/room_member.rs Outdated Show resolved Hide resolved
matrix_sdk_base/src/models/room_member.rs Outdated Show resolved Hide resolved
matrix_sdk_base/src/models/room_member.rs Outdated Show resolved Hide resolved
@martinetd martinetd force-pushed the direct_room branch 2 times, most recently from 0077ca6 to d3b4662 Compare October 8, 2020 13:27
@martinetd
Copy link
Contributor Author

Thank you, fixed the comments in their original commits as a force push (please say for future PRs if you prefer distinct fixing commits - I try to keep commits as small as possible while ensuring each commit is independently good to allow bisection)

I've also sneaked in some serialization test fixes.

Regarding the type length - you are indeed correct this is better on nightly, I've installed the toolchain to test and it passes.
However compilation is still awfully long (to my taste) -- for example the example login program takes over 10s to build on my laptop (build all, touch login.rs, build again).. As said multiple times I'm very new to rust so I don't really know what's normal but I a have similarily-sized toy hyper program that only takes ~2s to build (ruma uses hyper so it won't be faster but I'm not sure x4 is warranted?)
Well, I don't know. I'll still try to play with boxes and report when I figure out how to make this work -- but that's unrelated to this PR, so later :)

Until then I'm fine with the type length annotation, I thought it was for tests but it's the examples so I've added them in a new commit prior to breakage.

Some examples no longer build after the following commits, set a
bigger-than-default type_length_limit to let tests pass.

The exceptions are not necessary on nightly and can be removed again
after rust-lang/rust#54540 is fixed.
Rooms marked as "direct" are associated a user_id in "m.direct" events.
Clients could want to handle these separately
"m.direct" events are not in room account data events but in main one
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #104 into master will decrease coverage by 0.36%.
The diff coverage is 21.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
- Coverage   79.26%   78.90%   -0.37%     
==========================================
  Files          38       38              
  Lines        7563     7595      +32     
==========================================
- Hits         5995     5993       -2     
- Misses       1568     1602      +34     
Impacted Files Coverage Δ
matrix_sdk_base/src/models/message.rs 80.55% <ø> (ø)
matrix_sdk_base/src/models/room.rs 77.57% <0.00%> (-0.64%) ⬇️
matrix_sdk_base/src/models/room_member.rs 90.58% <ø> (ø)
matrix_sdk_base/src/state/mod.rs 91.89% <ø> (ø)
matrix_sdk_base/src/client.rs 66.55% <25.92%> (-2.71%) ⬇️
matrix_sdk_crypto/src/machine.rs 78.91% <0.00%> (-0.13%) ⬇️
matrix_sdk_crypto/src/olm/account.rs 85.38% <0.00%> (+0.45%) ⬆️
matrix_sdk_crypto/src/verification/sas/helpers.rs 77.41% <0.00%> (+0.46%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc48674...9ea4835. Read the comment docs.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, fixed the comments in their original commits as a force push (please say for future PRs if you prefer distinct fixing commits - I try to keep commits as small as possible while ensuring each commit is independently good to allow bisection)

I do dislike force pushing since it's a bit easier to see the changes if no force pushing is done.

Regarding the type length - you are indeed correct this is better on nightly, I've installed the toolchain to test and it passes.

As mentioned this seems to be fixed with Rust 1.47.0.

However compilation is still awfully long (to my taste) -- for example the example login program takes over 10s to build on my laptop (build all, touch login.rs, build again).. As said multiple times I'm very new to rust so I don't really know what's normal but I a have similarily-sized toy hyper program that only takes ~2s to build (ruma uses hyper so it won't be faster but I'm not sure x4 is warranted?)

I think that's the linking step taking this long, considering that there won't be much new stuff to compile.

Well, I don't know. I'll still try to play with boxes and report when I figure out how to make this work -- but that's unrelated to this PR, so later :)

I don't think boxing is the right approach here, using async syntax is the preferred way as the linked github issue also suggested.

matrix_sdk/examples/command_bot.rs Outdated Show resolved Hide resolved
This reverts commit 7d023eb.

Rust 1.47.0 got released and the tuning is no longer necessary.
@martinetd
Copy link
Contributor Author

I do dislike force pushing since it's a bit easier to see the changes if no force pushing is done.

I've got to say that's a github problem more than a force pushing problem. gerrit, gitlab etc do a great job with force push :P
I can leave fixup commits unsquashed and push normally then rebase/force push after you've reviewed new changes if you're ok with that?

I think that's the linking step taking this long, considering that there won't be much new stuff to compile.

Hard to tell with cargo but that appears correct (~4s for compilation and ~9 for linking); I guess changing things there won't help much with that..

I don't think boxing is the right approach here, using async syntax is the preferred way as the linked github issue also suggested.

It says async fn won't make deep types, but we obviously have the problem if we hit the 1MB limit on 'bugged' rust? I agree it was a performance regression/we shouldn't carry the length 'fix' now it's fixed but that we're hitting in in the first place makes me think we are generating deep types somewhere.
Anyway, let's drop that discussion for now, I didn't intend to take your time on that.
I've pushed a revert commit for the length type, let's see how travis fares.

@poljar poljar merged commit 9ea4835 into matrix-org:master Oct 8, 2020
@martinetd
Copy link
Contributor Author

Thanks!

@poljar
Copy link
Contributor

poljar commented Oct 8, 2020

Thanks for the PR.

@jplatte jplatte mentioned this pull request Oct 9, 2020
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.

Parse DirectEvent and expose info in Room struct
3 participants