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

Hf prewrite pause #19

Open
wants to merge 19 commits into
base: skycleaver
Choose a base branch
from
Open

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).

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