Skip to content

[Upstreaming] Added memory mapped file support #63

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

Closed
wants to merge 6 commits into from

Conversation

guptask
Copy link
Collaborator

@guptask guptask commented Apr 21, 2022

This change is Reviewable

@guptask guptask self-assigned this Apr 21, 2022
Copy link

@igchor igchor left a comment

Choose a reason for hiding this comment

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

@guptask Can you please cleanup commits and add some meaningful messages to them?

Also I think we are still missing support for temp shm mappings: https://github.com/pmem/CacheLib/blob/develop/cachelib/allocator/TempShmMapping.h as described in this issue:#26

We can probably send this patch to Meta as is and work on temp mapping separatly

Reviewable status: 0 of 21 files reviewed, all discussions resolved (waiting on @guptask)

@victoria-mcgrath
Copy link
Collaborator

cachelib/allocator/CacheAllocator.h line 1196 at r5 (raw file):

  static bool cleanupStrayShmSegments(
      const std::string& cacheDir, bool posix
      /*TODO: const std::vector<CacheMemoryTierConfig>& config = {} */);

Please consider using the type defined in CacheAllocatorConfg instead, e.g. CacheAllocatorConfig::MemoryTierConfigs

@victoria-mcgrath
Copy link
Collaborator

cachelib/allocator/CacheAllocator-inl.h line 95 at r5 (raw file):

  initCommon(false);
  shmManager_->removeShm(detail::kShmInfoName,
                         PosixSysVSegmentOpts(config_.usePosixShm));

Please consider calling the getter CacheAllocatorConfig::isUsingPosixShm() everywhere where you need to use usePosixShm.

@victoria-mcgrath
Copy link
Collaborator

cachelib/allocator/CacheAllocator-inl.h line 71 at r5 (raw file):

                          ShmSegmentOpts(config_.accessConfig.getPageSize(),
                                         false,
                                         config_.usePosixShm))

Would it make sense to use last two values as defaults for the last two parameters? Might help minimize the total number of changes .

@victoria-mcgrath
Copy link
Collaborator

cachelib/allocator/CacheAllocator-inl.h line 3530 at r5 (raw file):

    //   ShmManager::removeByName(cacheDir, tierShmName,
    //   config_.memoryTiers[i].opts);
    // }

Does thgis code need to be here? If yes, could you clean up the comment so it makes more sense in the absence contxet?

@victoria-mcgrath
Copy link
Collaborator

cachelib/allocator/TempShmMapping.cpp line 38 at r5 (raw file):

    if (addr_) {
      shmManager_->removeShm(detail::kTempShmCacheName.str(),
                             PosixSysVSegmentOpts(false /* posix */));

Make it false by default?

@victoria-mcgrath
Copy link
Collaborator

cachelib/shm/PosixShmSegment.h line 96 at r5 (raw file):

  // returns the key type corresponding to the given name.
  static std::string createKeyForName(const std::string& name) noexcept;

I was unable to find any code in your changes which required this method to be changed to public. Could you double check please?

@victoria-mcgrath
Copy link
Collaborator

cachelib/allocator/CacheAllocator-inl.h line 95 at r5 (raw file):

Previously, victoria-mcgrath wrote…

Please consider calling the getter CacheAllocatorConfig::isUsingPosixShm() everywhere where you need to use usePosixShm.

As A general observation, I am not sure I fully understand why we keep sending posix/non-posix as a parameter to many methods instead of setting this once per object and then just accessing it as a member.

Copy link
Collaborator

@victoria-mcgrath victoria-mcgrath left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r3, 6 of 16 files at r4, 14 of 14 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @guptask)


cachelib/allocator/CacheAllocator-inl.h line 71 at r5 (raw file):

minimize
reduce

@guptask guptask changed the title Added memory mapped file support [Upstreaming] Added memory mapped file support May 19, 2022
@guptask guptask closed this Jun 16, 2022
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

Successfully merging this pull request may close these issues.

3 participants