Skip to content

NAIF DAF mutation should be its own struct again #789

Description

@ChristopherRabotin

High level description

To mutate a DAF in a previous version (< 0.7?) one had to call to_mutable, which unlocked mutation functions. This would copy the underlying bytes to allow for mutation. This was removed in favor of a single DAF which allowed mutation.

With hindsight, that was a mistake.

The bytes were stored in a Bytes type, which is zero copy from the allocation point (or static &[u8]). Now, everything is stored in a BytesMut, which requires a Copy of the underlying bytes on initialization. A Clone operation is also a deep copy instead of a shallow pointer copy like Bytes.

That's a problem because the header of each file is read in the Almanac::load function to determine the file type, and if it's a DAF kind, the whole byte array is copied again to read the DAF. If the DAF was a Bytes then it would not need to copy the data again: the loading would be zerocopy.

This issue shows up in a flamegraph:

Image

The penalty is only paid once, but it should be paid zero times.

Requirements

  • Create a MutDAF struct, which has a freeze function that would return to an immutable DAF (or to_immutable()?)
  • The existing DAF functions that use a &mut self should be moved to the MutDAF struct.

Test plans

The existing tests should call my_daf.to_mutable() before trying to modify anything, and then freeze() before loading said file back in the Almanac.

Upload the flamegraph.svg from the built cargo test --release -- flamegraph and flamegraph -- ../target/release/.. -- flamegraph and the flamegraph command to show that the copy no longer happens. This proves the fix is correct.

Design

This design worked well before.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions