perf: faster package-lock tree creation by miiila · Pull Request #20 · snyk/nodejs-lockfile-parser

miiila · 2018-09-02T21:38:40Z

🚧 DO NOT MERGE 🚧

Tests written and linted ℹ︎
Documentation written ℹ︎
Commit history is tidy ℹ︎

What this does

Big package-lock.json parsing was slow. This PR introduces better parsing algorithm, which speeds up parsing a lot.

Instructions on how to run this locally, background context, what to review, questions…
Proper testing is still needed to be sure that functionality is the same. Tests are passing*, but they can't cover everything. However, it's pretty hard to verify what's right for the output with ~million lines.

* I am aware about one failing, it's a topic for the discussion about the meaning of depType property.

Before:

$ time parse-nodejs-lockfile > /dev/null
parse-nodejs-lockfile > /dev/null  30.25s user 1.24s system 123% cpu 25.522 total

After:

$ time node ../lockfile-parser/bin > /dev/null
node ../lockfile-parser/bin > /dev/null  0.39s user 0.06s system 103% cpu 0.436 total

30.25s vs 0.39s

michael-go · 2018-09-03T06:56:57Z

    }

+    // prepare a flat map, where keys are path, where dependencies are listed
+    const depMap: DepMap = await this.flattenLockfile(lockfile);


pretty sure all this async/await in this file is not needed. given no async IO is happening here, the async/await would still block the even-loop until done. So it only adds CPU overhead and complicates the code with various Promise.all()s and stuff

Yep, that's one of the thing I need your help and expertise - it's not needed now, but this package can end up being used in a deployed service, so we need to be sure we won't block event loop there. I know (from your talk) can push event loop to tick by setImmediate(), but I'd like to discuss the right approach.

michael-go · 2018-09-03T07:15:18Z

+
+      await Promise.all(_.entries(depQueue).map(async ([depKey, dep]) => {
+        let i = 0;
+        while (i < dep.dependenciesPathsToProcess.length) {


it's a while inside a Promise.all - map inside a while - maybe we can simplify it?

CLAassistant · 2018-10-03T19:52:00Z

All committers have signed the CLA.

Kirill89 · 2018-10-11T08:24:32Z

+    function setImmediatePromise() {
+      return new Promise((resolve, reject) => {
+        return setImmediate((err) => {
+          if (err) {


setImmediate can't call callback with error. So, function setImmediatePromise() { return new Promise(resolve => setImmediate(resolve)) }

michael-go · 2018-10-14T07:41:59Z

-      depTree.dependencies[dep.name] = await this.buildSubTreeRecursiveFromPackageLock(
-        dep.name, ['dependencies'], packageLock, [], dep.dev);
-    }));
+    // prepare a flat map, where dependency path is a key to dependency object


A comment about the if (._isEmpty(...)) above, unrelated to this PR: what will happen if both dependencies and devDepenedencies are empty?

It would bail out, but it's been fixed in #23

michael-go

This is really great 👏

I added mostly minor comments.

let's also add a test case where the cycle involves the root node (an example real pkg where it kinda happens: https://www.npmjs.com/package/trucolor)

michael-go · 2018-10-14T08:07:55Z

+
+  private createGraphOfDependencies(depMap: DepMap): graphlib.Graph {
+    const depGraph = new graphlib.Graph();
+    for (const depName of Object.keys(depMap)) {


it's better to name depName as depPath, like below, right? And actually depKey as used in flattenLockfile is even better I think.

michael-go · 2018-10-14T08:10:41Z

+  private createGraphOfDependencies(depMap: DepMap): graphlib.Graph {
+    const depGraph = new graphlib.Graph();
+    for (const depName of Object.keys(depMap)) {
+      depGraph.setNode(depName, depName);


the label (second param) is optional & I think you don't use it anywhere

michael-go · 2018-10-14T08:33:43Z

+    Once completed for all nodes in a cycle, original cyclic nodes can
+    be removed.
+  */
+


remove this blank line? 🙈 sorry

michael-go · 2018-10-14T08:42:13Z

+    const depGraph: graphlib.Graph = this.createGraphOfDependencies(depMap);
+
+    // topological sort will be applied and it requires acyclic graphs
+    let cycleStarts = {}; // cycle starts are need for top level dependencies


please try to improve this comment about cycleStarts. And typing it will help too.

michael-go · 2018-10-14T09:51:20Z

+      const newNode = start + uuid();
+      cycleStarts[start] = newNode;
+      depMap[newNode] = depMap[start];
+      // update depMap with new node


guess the comment is for one line above?

michael-go · 2018-10-14T19:57:51Z

+      // 1. Create a uniqe duplicate of entry node (without edges)
+      const newNode = start + uuid();
+      cycleStarts[start] = newNode;
+      depMap[newNode] = depMap[start];


why here the value is not _.cloneDeep-ed like done in similar operation in walkCycleRec() ?

michael-go · 2018-10-14T20:05:25Z

+        depTree.dependencies[dep.name] = dep.dev ?
+          this.setDevDepRec(_.cloneDeep(depTrees[depName])) : depTrees[depName];
+      }
+    }


would add the seImmediatePromise() call in this loop too, as the setDevDepRec might be costly (maybe call it only if dep.dev)

michael-go · 2018-10-14T20:19:28Z

+    for (const dep of topLevelDeps) {
+      // if any of top level dependencies is a part of cycle
+      // it now has a different item in the map
+      const depName = cycleStarts[dep.name] ? cycleStarts[dep.name] : dep.name;


can be simplified to:

const depName = cycleStarts[dep.name] || dep.name;

michael-go · 2018-10-14T20:26:28Z

+
+    if (!depMap[depName]) {
+      throw new Error(`Dependency ${depName} was not found in package-lock.json.
+        Your package.json and package-lock.json are probably out of sync.


when playing with some packages, this error happened to me not because the files were out-of-sync, but because one transitive dep failed to install. So this can happen sometimes not related to top-level deps. FYI

michael-go · 2018-10-14T20:30:56Z

+          depNode.requires = Object.keys(dep.requires);
+        }
+
+        const depKey: string[] = [...path, depName];


would call this depPath/depPathArray, and depKey would be the join-ed string

michael-go

Thanks for the improvements @miiila . approved this PR. Still worry a-little that this won't handle a case where there is a cycle that involves the root package, and it would be great if you can add a test case for this.

michael-go · 2018-10-26T14:05:44Z

      // Since this code doesn't handle any I/O or network, we need to force
      // event loop to tick while being used in server for request processing
-      await setImmediatePromise();
+      await this.setImmediatePromise();


it's a minor thing, but would personally prefer the setImmediatePromise to be a static or even global function as it doesn't use any internal state of the the class instance.

michael-go · 2018-10-26T14:14:34Z

+        To keep an order of algorithm steps readable, function is defined on-the-fly
+        Arrow function is used for calling `this` without .bind(this) in the end
+      */
+      const acyclicDuplicationRec = (node, traversed, currentCycle, nodeCopy) => {


this is great - and it will be even more readable if this definition moves 2 lines up, outside of the for loop. This way the algorithm can be read more outside-in.

michael-go · 2018-10-26T14:36:28Z

-      delete dep.requires;
-      depTrees[depKey] = {...dep as PkgTree};
+      const pkgTree: PkgTree = {
+        cyclic: dep.cyclic,


FYI, I see tests fail as cyclic & hasDevDependencies are always set although they are optional and sometimes not defined on dep.

miiila · 2018-11-02T17:47:40Z

@michael-go I did my best to add a test with self-reference cycle. This was so far the best I was able to create, because I'm not aware about any existing package, which would transitively require itself (trucolor doesn't do it anymore, it transitively requires older version of itself 😂); since it has an impact on how packages are hoisted, it's not easy to manually adjuste package-lock.json to simulate this situation.

So I created that kind of package by myself and algorithm doesn't break - it identifies cycle one level lower than it really is, but that's the limitation I'm happy to live with.

snyksec · 2018-11-05T09:34:07Z

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

miiila self-assigned this Sep 2, 2018

michael-go reviewed Sep 3, 2018

View reviewed changes

miiila force-pushed the perf/faster-npm-tree-parsing branch from b007b66 to 0effcfe Compare September 3, 2018 09:37

miiila force-pushed the perf/faster-npm-tree-parsing branch from 0effcfe to a9b6598 Compare October 3, 2018 19:51

miiila force-pushed the perf/faster-npm-tree-parsing branch from a9b6598 to 82d4476 Compare October 10, 2018 14:54

miiila requested review from Kirill89, adrukh, darscan, gjvis, lili2311 and robcresswell October 10, 2018 18:01

Kirill89 reviewed Oct 11, 2018

View reviewed changes

Comment thread lib/parsers/package-lock-parser.ts

miiila force-pushed the perf/faster-npm-tree-parsing branch 2 times, most recently from 2950aec to 15afacc Compare October 11, 2018 12:05

michael-go reviewed Oct 14, 2018

View reviewed changes

michael-go approved these changes Oct 26, 2018

View reviewed changes

Mila Votradovec added 8 commits November 2, 2018 16:44

perf: faster package-lock tree creation

489a4a8

chore: polishing, better naming, types etc

748e86a

chore: remove unnecesary asyncs + add one setImmediate

004449a

chore: better setImmediate

d0c6732

feat: keep dev dependency mark

44ac94b

chore: [].includes => _.includes

9695175

chore: more comments, better wording, better structure

59fd698

fix: proper setting of cyclic and dev deps, setImm. being function

e6ad71c

miiila force-pushed the perf/faster-npm-tree-parsing branch from 621ba49 to e6ad71c Compare November 2, 2018 16:45

fix: throw error for missing top level dep

9766c6e

test: cycle with self-reference

aa42ca2

miiila merged commit 1255d8f into master Nov 5, 2018

miiila deleted the perf/faster-npm-tree-parsing branch November 5, 2018 09:31

snyksec added the released label Nov 5, 2018

Uh oh!

Conversation

miiila commented Sep 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Uh oh!

michael-go Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michael-go Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-go left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-go Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-go Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-go left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-go Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miiila commented Nov 2, 2018

Uh oh!

snyksec commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

miiila commented Sep 2, 2018 •

edited

Loading

michael-go Sep 3, 2018 •

edited

Loading

CLAassistant commented Oct 3, 2018 •

edited

Loading

michael-go Oct 14, 2018 •

edited

Loading

michael-go Oct 14, 2018 •

edited

Loading

michael-go Oct 14, 2018 •

edited

Loading

michael-go left a comment •

edited

Loading

michael-go Oct 26, 2018 •

edited

Loading