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

Updated yrs to 0.18.2 #44

Merged
merged 8 commits into from
Apr 4, 2024
Merged

Updated yrs to 0.18.2 #44

merged 8 commits into from
Apr 4, 2024

Conversation

fbarbat
Copy link
Contributor

@fbarbat fbarbat commented Mar 28, 2024

This includes introducing a Subscription abstraction to get rid of the subscription IDs previously used that are not available anymore in Yrs.

#42

Package.swift Outdated Show resolved Hide resolved
lib/Cargo.toml Show resolved Hide resolved
@@ -184,23 +186,18 @@ impl YrsArray {
arr.remove_range(tx, index, len)
}

pub(crate) fn observe(&self, delegate: Box<dyn YrsArrayObservationDelegate>) -> u32 {
let subscription_id = self
pub(crate) fn observe(&self, delegate: Box<dyn YrsArrayObservationDelegate>) -> Arc<YSubscription> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this Arc return type. Again, I am not a Rust developer so I am not sure about the UniFFI generated code would not compile because it expected a type like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Horusiath what do you think about this? Do you think this is safe?

lib/src/subscription.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@heckj heckj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I'll greedily defer the Rust acceptability questions to @Horusiath , as feel very imposter every time I tackle something like that.

The two files in /scaffolding can be dumped from the PR, as they'll get regenerated by CI, but that doesn't strictly matter all that much - when we cut a release and punch in a new hash and XCFramework file during the release, they'll be regenerated as well.

I'll download the branch/PR and do some digging myself as to the memory leak. Off the top of my head, I suspect it's likely that because you made a YSubscription thing that passes over the Rust/Swift boundary, that this process is likely incurring a heavier cost, and I suspect related, but I'd need to dig a bit on the Swift side to see what I could find.

Package.swift Outdated Show resolved Hide resolved
@fbarbat
Copy link
Contributor Author

fbarbat commented Mar 29, 2024

Thank you @heckj! Reverted the files you mentioned.
Regarding the leak, it's worth noting that the leak was already there before this PR. Specifically, I ran this test in latest main and got the same behavior. So I am not sure my changes introduced the leak. As shown in commented out Rust prints in the PR, I verified my subscriptions were dropped. If I was doing something wrong please let me know! Thanks

import Foundation

import Combine
import XCTest
@testable import YSwift

class YSubscriptionTests: XCTestCase {
    var document: YDocument!
    var text: YText!
    
    override func setUp() {
        document = YDocument()
        text = document.getOrCreateText(named: "test")
    }
    
    override func tearDown() {
        document = nil
        text = nil
    }
    
    
    // This causes memory to keep growing and I am not sure why
    func test_allocateTooMany() {
        for i in 1...1000000000 {
            let s = text.observe { _ in }
            text.unobserve(s)
            
            if i % 1000000 == 0 {
                print("\(Date()) Iterations: \(i) Resident Size: \(residentSize() ?? 0)")
            }
        }
        
        print("Done!")
    }
    
    private func residentSize() -> UInt64? {
        var info = mach_task_basic_info()
        let MACH_TASK_BASIC_INFO_COUNT = MemoryLayout<mach_task_basic_info>.stride/MemoryLayout<natural_t>.stride
        var count = mach_msg_type_number_t(MACH_TASK_BASIC_INFO_COUNT)

        let kerr: kern_return_t = withUnsafeMutablePointer(to: &info) {
            $0.withMemoryRebound(to: integer_t.self, capacity: MACH_TASK_BASIC_INFO_COUNT) {
                task_info(mach_task_self_,
                          task_flavor_t(MACH_TASK_BASIC_INFO),
                          $0,
                          &count)
            }
        }

        guard kerr == KERN_SUCCESS else {
            return nil
        }
        
        return info.resident_size
    }
}

@fbarbat fbarbat requested a review from heckj March 29, 2024 00:48
@fbarbat
Copy link
Contributor Author

fbarbat commented Apr 2, 2024

@heckj exciting that this was approved! CI is failing though, how can we fix that?
Also, should we wait for @Horusiath comment about Rust Arc? All other conversations were resolved.

Working copy of https://github.com/y-crdt/yswift resolved at 0.2.0
error: local binary target 'yniffiFFI' at '/Users/runner/work/yswift/yswift/examples/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/yswift/lib/yniffiFFI.xcframework' does not contain a binary artifact.
error: fatalError
The 'swift' command exited with error code 1
Consider creating an issue using the following link: https://github.com/tuist/tuist/issues/new/choose
Error: Process completed with exit code 1.

@heckj
Copy link
Collaborator

heckj commented Apr 2, 2024

Back to fixing Tuist again? Damnit. I think it might be time to rip that out if it's not going to be stable. I'll work on getting a fix enabled there. And yeah, I'd still like @Horusiath 's input on that Arc question - I think it's OK (since it compiles, which is a kind of crap answer), but I just don't know

@heckj
Copy link
Collaborator

heckj commented Apr 2, 2024

I couldn't update your branch, so I've opened #46, which will hopefully flow though smoothly. If that works, we can rebase this over latest main and see how it goes. I'm not 100% that this solves things, but I'm hopeful.

@heckj
Copy link
Collaborator

heckj commented Apr 2, 2024

@fbarbat Rebase this on the main branch, and it should be happier now... and I probably screwed up the current main branch, so if you need or want to regenerate the scaffold files to sort the rebasing, go for it - we'll get it fully sorted when we cut a release with this included.

@fbarbat
Copy link
Contributor Author

fbarbat commented Apr 3, 2024

Rebased, kept your changes and updated the PR! Wouldn't it be nice to remove the generated files from GitHub, add them to git ignore, and make Xcode to generate them on build?

@heckj
Copy link
Collaborator

heckj commented Apr 3, 2024

Normally, I'd love to, but the content in the scaffold files is tightly coupled to the XCFramework binary that's generated for the same, so for someone "coming along" and just wanting to use the code, they need to be there. For releases, we update everything in lockstep. For latest development on the main branch, I'm not as chuffed if it's "broken" and requires a developer to use the YSWIFT_LOCAL along with building their own XCFramework

@heckj
Copy link
Collaborator

heckj commented Apr 3, 2024

All looks good with CI, tests passing, etc - so I'll hold on merging until you give me a thumbs up, in case you want to ask further questions. Once we merge this, I'll go ahead and do the leg-work to cut an updated release for more general consumption.

@fbarbat
Copy link
Contributor Author

fbarbat commented Apr 3, 2024

I just realized I pushed some generated files changes (bad merge probably). Should I remove those? You mentioned those are regenerated on release... What do you think?
On my side, it would be nice to have @Horusiath feedback on Rust Arc, otherwise all good. We've been running for 5 days this change on prod from a custom fork I created and everything is looking good.

@fbarbat
Copy link
Contributor Author

fbarbat commented Apr 3, 2024

Ok, I removed the changes to keep the PR cleaner. Can you run CI again? Thanks!

@heckj heckj merged commit 30892bf into y-crdt:main Apr 4, 2024
1 check passed
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.

3 participants