Skip to content

clip : use smart pointer (⚠️ breaking change) #12869

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 11, 2025
Merged

clip : use smart pointer (⚠️ breaking change) #12869

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9f2920d
clip : use smart pointers
ngxson Apr 9, 2025
4ab55e8
fix warmup
ngxson Apr 10, 2025
554256b
add forward declaration
ngxson Apr 10, 2025
39941d7
misisng include
ngxson Apr 10, 2025
f306d59
fix include (2)
ngxson Apr 10, 2025
f8c5394
composite
ngxson Apr 10, 2025
f727124
simplify batch ptr
ngxson Apr 10, 2025
fcc4608
Merge branch 'master' into xsn/clip_smart_ptr
ngxson Apr 11, 2025
6dbfb30
fix conflict
ngxson Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions examples/llava/clip-impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
#include "ggml.h"
#include "gguf.h"
#include "clip.h"

#include <climits>
#include <cstdarg>
#include <string>
#include <map>
#include <sstream>
#include <vector>
#include <memory>

// forward declarations
struct clip_image_u8;
struct clip_image_f32;
struct clip_image_size;

// Internal header for clip.cpp

Expand Down Expand Up @@ -178,6 +185,42 @@ static void clip_log_internal(enum ggml_log_level level, const char * format, ..
#define LOG_DBG(...) LOG_TMPL(GGML_LOG_LEVEL_DEBUG, __VA_ARGS__)
#define LOG_CNT(...) LOG_TMPL(GGML_LOG_LEVEL_CONT, __VA_ARGS__)

//
// cpp wrappers
//

struct clip_image_u8_deleter {
void operator()(clip_image_u8 * val) { clip_image_u8_free(val); }
};

struct clip_image_f32_deleter {
void operator()(clip_image_f32 * val) { clip_image_f32_free(val); }
};

struct clip_image_size_deleter {
void operator()(clip_image_size * val) { clip_image_size_free(val); }
};

struct clip_image_u8_ptr : std::unique_ptr<clip_image_u8, clip_image_u8_deleter> {
clip_image_u8_ptr() : std::unique_ptr<clip_image_u8, clip_image_u8_deleter>(clip_image_u8_init()) {}
};

struct clip_image_f32_ptr : std::unique_ptr<clip_image_f32, clip_image_f32_deleter> {
clip_image_f32_ptr() : std::unique_ptr<clip_image_f32, clip_image_f32_deleter>(clip_image_f32_init()) {}
};

typedef std::unique_ptr<clip_image_size, clip_image_size_deleter> clip_image_size_ptr;

// these need to be struct to maintain compatibility with C interface
struct clip_image_u8_batch : std::vector<clip_image_u8_ptr> {
clip_image_u8_batch() : std::vector<clip_image_u8_ptr>() {}
};
struct clip_image_f32_batch : std::vector<clip_image_f32_ptr> {
clip_image_f32_batch() : std::vector<clip_image_f32_ptr>() {}
~clip_image_f32_batch() {}
};
Copy link
Member

Choose a reason for hiding this comment

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

Inheritance from STL classes is generally not recommended. Although it's tempting, I think it would be better to avoid establishing this practice, as it can lead to subtle issues.

The recommended way is to use composition instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, that's good to know! (And I will also need to do the same on my llama_batch_ext PR)

With the help of gemini 3.5 pro, I added the composite in f8c5394

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about something much simpler like:

struct clip_image_u8_batch {
    std::vector<clip_image_u8_ptr> entries;
};

// ...
// end then using directly the `entries` member in the code
batch.entries.clear();
batch.entries.push_back(...);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes thanks, I simplify this in f727124

The clip_image_u8_ptr is also no longer being initialized by default, I think it's not a big deal, reduce the complexity of the wrapper

Copy link
Member

Choose a reason for hiding this comment

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

(And I will also need to do the same on my llama_batch_ext PR)

Yup, I will get back to this PR after some refactoring in libllama. We had a discussion with @slaren how to improve on it.

Copy link

Choose a reason for hiding this comment

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

Thanks, that's good to know! (And I will also need to do the same on my llama_batch_ext PR)

With the help of gemini 3.5 pro, I added the composite in f8c5394

Inheriting from std::unique_ptr is very strange usage of C++ pointers.

Also, std::shared_ptr might be a better option than std::unique_ptr as the former avoids dangling references. The overhead is usually not significant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tbh unique_ptr gives me more confident when writing code, kinda rust-like experience

Copy link

Choose a reason for hiding this comment

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

Tbh unique_ptr gives me more confident when writing code, kinda rust-like experience

No. They are not equivalent to rust semantics.

Copy link
Collaborator Author

@ngxson ngxson Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I said rust-like experience, not rust-equivalent semantics. unique_ptr is useful in my case because the compiler will complain about unique_ptr being non-copiable, so I have total control over the life cycle of the object owned by unique_ptr

Copy link

@acbits acbits Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense. Just be aware of
.get() usage as it could create dangling pointers despite unique ownership.



//
// common utils
//
Expand Down
Loading
Loading