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

aya-obj: Mutate BTF in-place without clone #731

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

dave-tucker
Copy link
Member

@dave-tucker dave-tucker commented Aug 9, 2023

The BTF we're working on is Cow anyway so modifying in-place is fine. All we need to do is store some information before we start our mutable iteration to avoid concurrently borrowing types both mutably and immutably.


This change is Reviewable

@netlify
Copy link

netlify bot commented Aug 9, 2023

Deploy Preview for aya-rs-docs ready!

Name Link
🔨 Latest commit 098d436
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64d397abddddb20008b685ff
😎 Deploy Preview https://deploy-preview-731--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify mergify bot added aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI labels Aug 9, 2023
Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dave-tucker)


aya-obj/src/btf/btf.rs line 531 at r1 (raw file):

                    let mut members = vec![];
                    for member in d.entries.iter() {

so i think we can avoid the need for datasec_member_name_offset here if we realize that we're about to throw away d, so we can write:

let entries = std::mem::take(d.entries);
// now we've dropped the mutable borrow of types.types, i hope
//
// populate members as before
types.types[i] = ...


aya-obj/src/btf/btf.rs line 584 at r1 (raw file):

                        for e in &mut d.entries.iter_mut() {
                            let (name_offset, linkage) =
                                var_types.get(&e.btf_type).ok_or(BtfError::InvalidDatasec)?;

i think we can do similar tricks here; hollow out d instead of cloning it, which drops the mutable reference, and then write a new one. that avoids var_types which is also allocates.


aya-obj/src/btf/btf.rs line 1256 at r1 (raw file):

        )));

        let name_offset = btf.add_string("foo");

should we extract local for this "foo", or perhaps assert that name_offset1 is equal to this name_offset below?


aya-obj/src/btf/types.rs line 868 at r1 (raw file):

#[repr(C)]
#[derive(Clone, Debug)]

do we want to remove these clone impls?


aya-obj/src/btf/types.rs line 877 at r1 (raw file):

impl DataSec {
    pub fn name_offset(&self) -> u32 {

why are these getters needed? the fields are pub(crate).

The BTF we're working on is Cow anyway so modifying in-place is fine.
All we need to do is store some information before we start our
mutable iteration to avoid concurrently borrowing types both mutably and
immutably.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
Copy link
Member Author

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @tamird)


aya-obj/src/btf/btf.rs line 531 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

so i think we can avoid the need for datasec_member_name_offset here if we realize that we're about to throw away d, so we can write:

let entries = std::mem::take(d.entries);
// now we've dropped the mutable borrow of types.types, i hope
//
// populate members as before
types.types[i] = ...

Done.


aya-obj/src/btf/btf.rs line 584 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i think we can do similar tricks here; hollow out d instead of cloning it, which drops the mutable reference, and then write a new one. that avoids var_types which is also allocates.

Done.


aya-obj/src/btf/btf.rs line 1256 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

should we extract local for this "foo", or perhaps assert that name_offset1 is equal to this name_offset below?

Done.


aya-obj/src/btf/types.rs line 868 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

do we want to remove these clone impls?

We can't because Btf is Clone on write. So all everything contained within also needs to be clone since we derive the clone impl.


aya-obj/src/btf/types.rs line 877 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why are these getters needed? the fields are pub(crate).

Done.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dave-tucker)


aya-obj/src/btf/btf.rs line 511 at r2 (raw file):

                    let members = entries
                        .iter()
                        .map(|e| {

Can "e" be "member"? Reduces the diff.


aya-obj/src/btf/btf.rs line 562 at r2 (raw file):

                        // we can query by name in symbol_offsets
                        let mut entries = mem::take(&mut d.entries);
                        let mut fixed_section = d.clone();

Is this needed because there's a private field?


aya-obj/src/btf/btf.rs line 564 at r2 (raw file):

                        let mut fixed_section = d.clone();

                        for e in entries.iter_mut() {

s/e/d/ to match old name and reduce diff

Copy link
Member Author

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tamird)


aya-obj/src/btf/btf.rs line 562 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Is this needed because there's a private field?

Yes. But at least we've pulled out the Vec now.

@dave-tucker dave-tucker merged commit e210012 into aya-rs:main Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya-obj Relating to the aya-obj crate test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants