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.

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

Jump to bottom

idea: move logic from fn metadata(&self) -> Arc<AccessorInfo> to impl<A: Access> Layer<A> for CompleteLayer #4888

Open
1 task done
Lzzzzzt opened this issue Jul 13, 2024 · 3 comments · May be fixed by #4896
Open
1 task done

idea: move logic from fn metadata(&self) -> Arc<AccessorInfo> to impl<A: Access> Layer<A> for CompleteLayer #4888

Lzzzzzt opened this issue Jul 13, 2024 · 3 comments · May be fixed by #4896
Labels
core enhancement New feature or request

Comments

@Lzzzzzt
Copy link
Contributor

Lzzzzzt commented Jul 13, 2024

Feature Description

Since the CompleteAccessor stores the metadata, and Access::info return the Arc<AccessorInfo>, we can move logic from fn metadata(&self) -> Arc<AccessorInfo> to impl<A: Access> Layer<A> for CompleteLayer to avoid creating a new Arc<AccessorInfo>(this is not add the reference count).

Problem and Solution

just change CompleteAccessor::metadata:

fn metadata(&self) -> Arc<AccessorInfo> {
    let mut meta = (*self.meta).clone();
    let cap = meta.full_capability_mut();
    if cap.list && cap.write_can_empty {
        cap.create_dir = true;
    }
    meta.into()
}

to

fn metadata(&self) -> Arc<AccessorInfo> {
   self.meta.clone()
}

and change CompleteLayer::layer:

fn layer(&self, inner: A) -> Self::LayeredAccess {
    CompleteAccessor {
        meta: inner.info(),
        inner: Arc::new(inner),
    }
}

to

fn layer(&self, inner: A) -> Self::LayeredAccess {
    let mut meta = inner.info().as_ref().clone();
    let cap = meta.full_capability_mut();

    if cap.list && cap.write_can_empty {
        cap.create_dir = true;
    }

    CompleteAccessor {
        meta: meta.into(),
        inner: Arc::new(inner),
    }
}

Additional Context

All the layers that store the metadata can apply this change.

Are you willing to contribute to the development of this feature?

  • Yes, I am willing to contribute to the development of this feature.
@Lzzzzzt Lzzzzzt added the enhancement New feature or request label Jul 13, 2024
@Xuanwo Xuanwo added the core label Jul 13, 2024
@Lzzzzzt
Copy link
Contributor Author

Lzzzzzt commented Jul 13, 2024

should these accessor like BlockingAccessor that doesn't store the metadata but change the capability add a field to store the new metadata to avoid needless memory allocation.

opendal/core/src/layers/blocking.rs

Lines 183 to 187 in 31d0967

fn metadata(&self) -> Arc<AccessorInfo> {
let mut meta = self.inner.info().as_ref().clone();
meta.full_capability_mut().blocking = true;
meta.into()
}

@Xuanwo
Copy link
Member

Xuanwo commented Jul 13, 2024 •
edited
Loading

Looks good to me, we can construct the info during build time.

@Lzzzzzt Lzzzzzt linked a pull request Jul 14, 2024 that will close this issue
Draft
@Lzzzzzt
Copy link
Contributor Author

Lzzzzzt commented Jul 14, 2024

It seems that after apply this change, the c binding CI test will fail, but I don't know why this happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
core enhancement New feature or request
Projects
None yet
Milestone
No milestone
2 participants
@Xuanwo @Lzzzzzt