Skip to content

Conversation

larixer
Copy link
Member

@larixer larixer commented Jun 7, 2022

What's the problem this PR addresses?

This PR implements content-addressable storage support for pnpm linker via nmMode: hardlinks-global config setting.

Integrates PR: #4532

How did you fix it?

The PR is reusing hardlinks-global implementation from node-modules linker.

pnpm linker link step timings on one of my rather big projects:

  1. From master (no hardlinks store): 18.9 secs
  2. From this PR (no hardlinks store): 14.8 secs
  3. From this PR (nmMode: hardlinks-global, empty store): 37.8 secs
  4. From this PR (nmMode: hardlinks-global, filled store): 15.2 secs, it is a bit slower than wihout hardlinks, because content addressable store needs more fs operations on packages without checksums, which is the case for conditional packages like fsevents

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@larixer larixer marked this pull request as ready for review June 8, 2022 05:36
Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

I want to take some time to think about this; I may experiment myself with implementing some logic into the main copyPromise (perhaps it won't work anyway, but I'd like to better understand why).

} | {
kind: DirEntryKind. DIRECTORY;
} | {
kind: DirEntryKind.SYMLINK;
symlinkTo: PortablePath;
};

const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs, globalHardlinksStore, nmMode, packageChecksum}: {baseFs: FakeFS<PortablePath>, globalHardlinksStore: PortablePath | null, nmMode: {value: NodeModulesMode}, packageChecksum: string | null}) => {
export const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs, hardlinksStorePath, nmMode, packageChecksum}: {baseFs: FakeFS<PortablePath>, hardlinksStorePath: PortablePath | null, nmMode: {value: NodeModulesMode}, packageChecksum: string | null}) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm really not fan of having a separate copy function... It was fine when it was bound to the nm linker, but if it leaks into the others it starts to become concerning imo 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Please review #4532 then, I've reopened it

@TAUnionOtto

This comment was marked as off-topic.

@larixer
Copy link
Member Author

larixer commented Oct 13, 2022

Closed in favour of: #4586, although the latter has worse performance

@larixer larixer closed this Oct 13, 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