Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

Well managed file descriptor and mmap-ed memory #454

Merged
merged 8 commits into from
Sep 24, 2019

Conversation

primenumber
Copy link
Contributor

Implement class for manage file descriptor / mmap-ed memory for dlk.

Motivation and Context

In current implementation, file descriptor and mmap-ed memory are not managed, so resource leakage occurs.
This PR fixes it.

Description

To manage file descriptor, FDManager
To manage mmap-ed memory, SimpleMappedMem

I considered to use std::unique_ptr:
Type of file descriptor is not pointer, so we have to implement custom class for store file descriptor which can be compare with std::nullptr_t.
munmap() require mapped size, so we have to implement custom class for store pointer with mapped size.
These increase implement cost.

How has this been tested?

$ PYTHONPATH=python/dlk python python/dlk/scripts/generate_project.py -i minimal_graph_with_shape.pb -o tmp/ -p object_detection -hq -ts -cache
$ cd tmp/object_detection.prj
$ mkdir build
$ cd build
$ cmake .. -DTOOLCHAIN_NAME=linux_arm -DUSE_NEON=1 -DRUN_ON_FPGA=1
$ make -j8 lm

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature / Optimization (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@primenumber primenumber self-assigned this Sep 20, 2019
@blueoil-butler blueoil-butler bot added the CI: auto-run Run CI automatically label Sep 20, 2019
#include <memory>
#include <system_error>

class FDManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

For me, this FDManager class represents file descriptor itself, rather than a manager. How about renaming this class to just FD or FileDescriptor or something?

If you are uncomfortable using such a common name, you can also use namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed.

int fd;
};

class SimpleMappedMem {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since MappedMem class seems not be used in the current code, how about removing current MappedMem class and renaming this SimpleMappedMem class to MappedMem class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@tkng tkng left a comment

Choose a reason for hiding this comment

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

  • readability approval
  • ownership approval

@tkng tkng merged commit a7b22b7 into blue-oil:master Sep 24, 2019
@primenumber primenumber deleted the feature/fd-manager branch September 25, 2019 02:40
ananno pushed a commit to ananno/blueoil that referenced this pull request Sep 27, 2019
* implement FileDescriptor class, rewrite MappedMem class to RAII style
@iizukak iizukak added this to the v0.12.0 milestone Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI: auto-run Run CI automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants