Skip to content

Conversation

@hefehr
Copy link
Collaborator

@hefehr hefehr commented Sep 20, 2024

As requested, I create the pull request into the skycleaver branch.

@hefehr
Copy link
Collaborator Author

hefehr commented Sep 20, 2024

I wasn't quite able to use unique_ptrs. I got the error message:

/usr/src/skyweaver/cpp/skyweaver/detail/BeamformerPipeline.cu(117): error: class "std::unique_ptr<skyweaver::MultiFileWriter<skyweaver::DescribedVector<thrust::THRUST_200302_860_890_NS::host_vector<int8_t, thrust::THRUST_200302_860_890_NS::mr::stateless_resource_allocator<int8_t, skyweaver::MemoryResource>>, skyweaver::BeamDim, skyweaver::TimeDim, skyweaver::FreqDim>>, std::default_delete<skyweaver::MultiFileWriter<skyweaver::DescribedVector<thrust::THRUST_200302_860_890_NS::host_vector<int8_t, thrust::THRUST_200302_860_890_NS::mr::stateless_resource_allocator<int8_t, skyweaver::MemoryResource>>, skyweaver::BeamDim, skyweaver::TimeDim, skyweaver::FreqDim>>>>" has no member "init"
      _ib_handler.init(_header);

Apart from this, it seems to work.

@hefehr
Copy link
Collaborator Author

hefehr commented Sep 25, 2024

Work now with unique_ptrs. The code aborts now without an active exception.

[2024-Sep-25 10:38:55.178455] [debug] [...BeamformerPipeline::process] Maximum allowed file size = 9999999959040 bytes (+header)
[2024-Sep-25 10:38:55.178531] [debug] [...BeamformerPipeline::process] Creating output file stream with parameters,
Output directory: /2024-06-05-02:28:08/0/718250000
Base filename: 2024-06-05-02:28:08_0_01536.000_ib_718250000
Extension: .btf
Number of bytes per file: 9999999959040
terminate called without an active exception
Aborted

@vivekvenkris
Copy link
Member

vivekvenkris commented Sep 25, 2024

HI @hefehr and @ewanbarr,

I am confused.

We previously had the handler instances created on stack like this:

    IBWriterType ib_handler(config, "ib", create_stream_callback_ib);

which was fine to do because it is not invalidated until the entire pipeline is run.

Now the code has changed it to unique pointers in the declaration but still uses *ib_handler.get() to get the raw pointer to the pipeline() call. What is the point of doing unique pointers then?

We should either change all subsequent declarations to use unique_ptrs or not use them at all.

@ewanbarr
Copy link
Contributor

You can't conditionally allocate non-copyable, non-moveable types on the stack.

@vivekvenkris
Copy link
Member

Ewan and I discussed and I get it now, I am going to merge skycleaver to pipeline_dev now, and then pull this into skycleaver, as part of the next roll off of features.

@ewanbarr
Copy link
Contributor

ewanbarr commented Sep 28, 2024

Btw, I realised that my previous statement is not quite right. Since C++17 you can do this using immediately invoked function expressions (IIFEs). Basically a lambda that is called at construction. C++17 and above guarantee that return value optimisation (RVO) is used in these circumstances, so the compiler will elide the function call.

struct NonCopyableNonMovable {
    NonCopyableNonMovable(int x) { std::cout << "Constructor called with x: " << x << std::endl; }
    NonCopyableNonMovable(int x, int y) { std::cout << "Constructor called with x: " << x << ", y: " << y << std::endl; }

    // Delete copy/move constructors
    NonCopyableNonMovable(const NonCopyableNonMovable&) = delete;
    NonCopyableNonMovable(NonCopyableNonMovable&&) = delete;
    NonCopyableNonMovable& operator=(const NonCopyableNonMovable&) = delete;
    NonCopyableNonMovable& operator=(NonCopyableNonMovable&&) = delete;
    ~NonCopyableNonMovable() = default;
};

int main() {
    bool condition = true;  // Some condition

    NonCopyableNonMovable obj = [&]() -> NonCopyableNonMovable {
        if (condition) {
            return NonCopyableNonMovable{42};  // Constructor for one argument
        } else {
            return NonCopyableNonMovable{10, 20};  // Constructor for two arguments
        }
    }();

    // Use obj here...
    return 0;
}

This can be used instead of the unique_ptr if desired (doesn't materially change anything, just means that the object is stack allocated which is a bit faster).

coclar and others added 26 commits January 29, 2025 15:29
Optional output of stats and incoherent beam files
Fix integer overflow in SkyCleaver's expected bridge frequencies
@hefehr
Copy link
Collaborator Author

hefehr commented May 8, 2025

Ewan and I discussed and I get it now, I am going to merge skycleaver to pipeline_dev now, and then pull this into skycleaver, as part of the next roll off of features.

Hi @ewanbarr and @vivekvenkris,
I'd like to resolve the merge conflicts and test the code, since we start to need this functionality. Could we still implement it?

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.

5 participants