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

OperatorInfo should be stored inside Operator to avoid calling metadata #4845

Open
Xuanwo opened this issue Jul 2, 2024 · 11 comments
Open

Comments

@Xuanwo
Copy link
Member

Xuanwo commented Jul 2, 2024

OperatorInfo will never be changed after Operator been built, we can store it inside an Arc inside Operator to avoid extra cost.

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 11, 2024

If my understanding is right, it just wrap OperatorInfo in Arc<T>.

And if it is better to use OnceCell<T> for lazy init of OperatorInfo and change pub fn info(&self) -> OperatorInfo to pub fn info(&self) -> &OperatorInfo?

@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 12, 2024

Hi, @Lzzzzzt. Thanks for joining the discussion and starting #4883.

After some consideration, I think it would be better to modify Access::info() to return Arc<AccessInfo>, similiar to what you suggested in #4883 (comment). This will allow all our layers to avid extra copy.

With this change, OperatorInfo could hold an Arc<AccessInfo>, allowing the user API to remain unchanged.

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 12, 2024

Hi, @Lzzzzzt. Thanks for joining the discussion and starting #4883.

After some consideration, I think it would be better to modify Access::info() to return Arc<AccessInfo>, similiar to what you suggested in #4883 (comment). This will allow all our layers to avid extra copy.

With this change, OperatorInfo could hold an Arc<AccessInfo>, allowing the user API to remain unchanged.

Ok, got it

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 12, 2024

Hi, @Lzzzzzt. Thanks for joining the discussion and starting #4883.

After some consideration, I think it would be better to modify Access::info() to return Arc<AccessInfo>, similiar to what you suggested in #4883 (comment). This will allow all our layers to avid extra copy.

With this change, OperatorInfo could hold an Arc<AccessInfo>, allowing the user API to remain unchanged.

It seems need to change a lot, if modify Access::info() to return Arc<AccessInfo>

@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 12, 2024

It seems need to change a lot, if modify Access::info() to return Arc<AccessInfo>

Yes. If we do need API breaking changes, I prefer change opendal::raw API instead of opendal's public API.

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 12, 2024

It seems need to change a lot, if modify Access::info() to return Arc<AccessInfo>

Yes. If we do need API breaking changes, I prefer change opendal::raw API instead of opendal's public API.

if modify Access::info() to return Arc, a lot of struct will need a extra field to store the info, is that right?

fn info(&self) -> AccessorInfo {
let mut am = AccessorInfo::default();
am.set_scheme(Scheme::Fs)
.set_root(&self.core.root.to_string_lossy())
.set_native_capability(Capability {
stat: true,
read: true,
write: true,
write_can_empty: true,
write_can_append: true,
write_can_multi: true,
create_dir: true,
delete: true,
list: true,
copy: true,
rename: true,
blocking: true,
..Default::default()
});
am
}

@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 12, 2024

if modify Access::info() to return Arc, a lot of struct will need a extra field to store the info, is that right?

We can start by simply adding an into() and refactor them later based on our feedback.

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 12, 2024

some layers need a mutable AccessorInfo, so Arc<T> may not work

impl<A: Access> LayeredAccess for BlockingAccessor<A> {
type Inner = A;
type Reader = A::Reader;
type BlockingReader = BlockingWrapper<A::Reader>;
type Writer = A::Writer;
type BlockingWriter = BlockingWrapper<A::Writer>;
type Lister = A::Lister;
type BlockingLister = BlockingWrapper<A::Lister>;
fn inner(&self) -> &Self::Inner {
&self.inner
}
fn metadata(&self) -> AccessorInfo {
let mut meta = self.inner.info();
meta.full_capability_mut().blocking = true;
meta
}

@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 12, 2024

some layers need a mutable AccessorInfo, so Arc<T> may not work

Those layers can build a new AccessorInfo with changed value.

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 12, 2024

some layers need a mutable AccessorInfo, so Arc<T> may not work

Those layers can build a new AccessorInfo with changed value.

ok

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 12, 2024

I change the signature of Access::info and AccessDyn::info and related layers implement, and fs service.
PTAL

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

No branches or pull requests

2 participants