Skip to content
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

perf: integrate nimma #1136

Merged
merged 20 commits into from
May 29, 2020
Merged

perf: integrate nimma #1136

merged 20 commits into from
May 29, 2020

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented May 3, 2020

Closes #437, #506

Similarly to #1112, I'm going to extract a few unrelated changes to a separate PR.

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

Introduces a totally different approach to JSON Path expression-based object lookup, leveraging nimma. Note - we still do rely on jsonpath-plus in certain circumstances, as nimma does not support the entire JSON Path syntax and all jsonpath-plus additions. The coverage is good enough, however, and almost the whole oas ruleset have expressions that can be optimized.
Currently, nimma is disabled by default and will be used only in Studio where performance is critical.
I'll enable it in future for everyone.
There are still a few changes left to make:

  • $ref resolver - I've got a project that is slowly shaping up - once we switch to it, we should observe further improvements,
  • oas2/oas3 schema rules are slow hogs,
  • nimma itself could be optimized and have better coverage of JSON Path syntax.

Screenshots

openapi-directory/APIs/stripe.com/2019-09-09/swagger.yaml

This PR using nimma
image

5.4.0-beta1
image

5.4.0-beta1 with expensive rules disabled
image


openapi-directory/APIs/sendgrid.com/3.0/swagger.yaml

This PR using nimma
image

5.4.0-beta1
image

5.4.0-beta1 with expensive rules disabled
image


openapi-directory/APIs/adyen.com/**/*.yaml

This PR using nimma
image

5.4.0-beta1
image

5.4.0-beta1 with expensive rules disabled
image


openapi-directory/APIs/stoplight.io/api-v1/swagger.yaml

This PR using nimma
image

5.4.0-beta1
image

5.4.0-beta1 with expensive rules disabled
image

@P0lip P0lip self-assigned this May 3, 2020
@P0lip P0lip added the chore label May 3, 2020
src/runner/linter.ts Outdated Show resolved Hide resolved
src/runner/linter.ts Outdated Show resolved Hide resolved
@nulltoken
Copy link
Contributor

@P0lip Impressive gains! 👏

Currently, nimma is disabled by default and will be used only in Studio where performance is critical.
I'll enable it in future for everyone.

How about tweaking the tests so that they run twice, with and without Nimma?

@P0lip
Copy link
Contributor Author

P0lip commented May 4, 2020

How about tweaking the tests so that they run twice, with and without Nimma?

Agree.
How do you feel about running suites twice? Once without Nimma, and once with Nimma.
Thinking of something as follows

"test": "jest && NIMMA=true jest"

@nulltoken
Copy link
Contributor

How do you feel about running suites twice? Once without Nimma, and once with Nimma.
Thinking of something as follows

"test": "jest && NIMMA=true jest"

👍 I was thinking along those lines as well (as this dual setup is temporary).
Maybe also let karma know about this?

@marbemac
Copy link
Contributor

marbemac commented May 8, 2020

Looks very promising!

@P0lip P0lip mentioned this pull request May 26, 2020
4 tasks
@P0lip P0lip force-pushed the perf/json-paths-on-rails branch 2 times, most recently from 8cad254 to 5c120e8 Compare May 26, 2020 02:17
@P0lip
Copy link
Contributor Author

P0lip commented May 26, 2020

This should be good to go once #1184 is merged

@P0lip P0lip changed the base branch from develop to perf/core-functions May 26, 2020 19:08
@P0lip P0lip requested a review from nulltoken May 26, 2020 19:08
@P0lip P0lip marked this pull request as ready for review May 26, 2020 19:08
package.json Show resolved Hide resolved
src/rule.ts Outdated Show resolved Hide resolved
let piece = document;

for (let i = 0; i < path.length - 1; i += 1) {
if (!isObject(piece)) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip I know this is subject to debate, but I'd be in the camp of always adding braces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nulltoken we can actually introduce some eslint rule.
I have no strong preference, but if you like curly braces, I'm all in for introducing the rule, so curly braces are always enforced.

src/rulesets/oas/functions/refSiblings.ts Outdated Show resolved Hide resolved
src/rulesets/oas/functions/refSiblings.ts Outdated Show resolved Hide resolved
src/rulesets/oas/functions/refSiblings.ts Outdated Show resolved Hide resolved
src/rulesets/oas/functions/refSiblings.ts Outdated Show resolved Hide resolved
src/rulesets/oas/functions/typedEnum.ts Outdated Show resolved Hide resolved
src/rulesets/oas/index.json Outdated Show resolved Hide resolved
Base automatically changed from perf/core-functions to develop May 27, 2020 11:46
@P0lip P0lip force-pushed the perf/json-paths-on-rails branch from df54aa2 to 0d71a61 Compare May 28, 2020 02:04
@nulltoken
Copy link
Contributor

@P0lip Minor refactoring proposal which should add to the jest and karma logs in which context the tests are being run. Thoughts?

diff --git a/jest.config.js b/jest.config.js
index ebb7d9e..047ff91 100644
--- a/jest.config.js
+++ b/jest.config.js
@@ -5,5 +5,6 @@ module.exports = {
   testMatch: ['<rootDir>/src/**/__tests__/*.(ts)'],
   testPathIgnorePatterns: ['/node_modules/', '\.karma\.test\.ts$'],
   coveragePathIgnorePatterns: ['<rootDir>/dist/', '/node_modules/'],
-  setupFilesAfterEnv: ['./setupJest.ts', './setupTests.ts']
+  setupFilesAfterEnv: ['./setupJest.ts', './setupTests.ts'],
+  globalSetup: '<rootDir>/setupJest.global.js',
 };
diff --git a/karma-jest.ts b/karma-jest.ts
index 9d5f656..5254988 100644
--- a/karma-jest.ts
+++ b/karma-jest.ts
@@ -1,5 +1,6 @@
 import { Expect } from 'expect/build/types';
 import * as JestMock from 'jest-mock';
+import { isNimmaEnvVariableSet } from './src/utils/isNimmaEnvVariableSet'
 
 declare var global: NodeJS.Global & {
   jest: typeof JestMock;
@@ -7,6 +8,8 @@ declare var global: NodeJS.Global & {
   test: jest.It;
 };
 
+console.info(`Nimma rule optimizer activated: ${isNimmaEnvVariableSet()}`);
+
 global.jest = require('jest-mock');
 global.expect = require('expect');
 global.test = it;
diff --git a/setupJest.global.js b/setupJest.global.js
new file mode 100644
index 0000000..71d2298
--- /dev/null
+++ b/setupJest.global.js
@@ -0,0 +1,7 @@
+'use strict';
+
+const isNimmaEnvVariableSet = require('./src/utils/isNimmaEnvVariableSet').isNimmaEnvVariableSet;
+
+module.exports = async () => {
+  console.info(`Nimma rule optimizer activated: ${isNimmaEnvVariableSet()}`);
+};
diff --git a/src/spectral.ts b/src/spectral.ts
index 3260531..6fb5881 100644
--- a/src/spectral.ts
+++ b/src/spectral.ts
@@ -33,6 +33,7 @@ import {
 import { IRuleset, RulesetExceptionCollection } from './types/ruleset';
 import { ComputeFingerprintFunc, defaultComputeResultFingerprint, empty } from './utils';
 import { generateDocumentWideResult } from './utils/generateDocumentWideResult';
+import { isNimmaEnvVariableSet } from './utils/isNimmaEnvVariableSet';
 
 memoize.Cache = WeakMap;
 
@@ -133,13 +134,7 @@ export class Spectral {
     empty(this.rules);
 
     for (const [name, rule] of Object.entries(rules)) {
-      if (
-        this.opts?.useNimma ||
-        (typeof global === 'object' && global?.process?.env?.USE_NIMMA) ||
-        // eslint-disable-next-line @typescript-eslint/ban-ts-ignore
-        // @ts-ignore, this is for Karma
-        (typeof window === 'object' && window?.__env__?.USE_NIMMA)
-      ) {
+      if (this.opts?.useNimma || isNimmaEnvVariableSet()) {
         try {
           this.rules[name] = new OptimizedRule(name, rule);
         } catch {
diff --git a/src/utils/isNimmaEnvVariableSet.ts b/src/utils/isNimmaEnvVariableSet.ts
new file mode 100644
index 0000000..736518a
--- /dev/null
+++ b/src/utils/isNimmaEnvVariableSet.ts
@@ -0,0 +1,11 @@
+export const isNimmaEnvVariableSet = (): boolean => {
+  const globalNimma = typeof global !== 'undefined' && typeof global === 'object' && Boolean(global?.process?.env?.USE_NIMMA);
+
+  // eslint-disable-next-line @typescript-eslint/ban-ts-ignore
+  // @ts-ignore
+  const windowNimma = typeof window !== 'undefined' && typeof window === 'object' && Boolean(window?.__env__?.USE_NIMMA);
+
+  const isNimmaActivated = globalNimma || windowNimma;
+
+  return Boolean(isNimmaActivated);
+};
-- 

@P0lip
Copy link
Contributor Author

P0lip commented May 28, 2020

@nulltoken I like it!

@P0lip P0lip requested a review from nulltoken May 28, 2020 22:28
Copy link
Contributor

@nulltoken nulltoken left a comment

Choose a reason for hiding this comment

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

:shipit: ‼️

@P0lip P0lip merged commit 240e07e into develop May 29, 2020
@P0lip P0lip deleted the perf/json-paths-on-rails branch May 29, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Spectral performance from O(n^2) to O(n)
3 participants