Performance

[CacheControl]

Alpha candidate: 2.2.0-alpha1

• Use standard promises instead of async/await; saves due to transpilation performance
• Constant facts now skip the caching logic; saves significant cpu due to cache key calculation
• Removal of json.stringify from debug statements; improves large object performance

[hartzis]

nice!

[ferrants]

@CacheControl , if I were to test these changes out and post back about how they performed, would that help you merge this? It would probably be in the form of, "This file took 8 hours to process before and now takes 3 hours", would that be helpful? Any other specific information I can provide in testing this? We pass a file through the engine, passing each record's fields as constant facts to the engine.

[ferrants]

Not sure where else some logic had to change, it looks like the allowUndefinedFacts isn't respected in this version. I get this error:

Error: facts must have a value or method
      at new Fact (node_modules/json-rules-engine/dist/fact.js:46:13)
      at new Almanac (node_modules/json-rules-engine/dist/almanac.js:47:16)
      at Engine.run (node_modules/json-rules-engine/dist/engine.js:249:21)
      at Context.it (test/services/RulesService.test.js:35:21)

Reproduced by:

const engine = new Engine([{
  conditions: {
    all: [
      {
        fact: 'something',
        operator: 'equal',
        value: 'foo'
      }
    ]
  },
  event: {
    type: 'bar',
    params: {
      other: 'baz'
    }
  }
}], {
  allowUndefinedFacts: true
});

return engine.run({
  something: undefined,
  somethingElse: 'foo'
})
.then(events => {
  expect(events).to.have.length(0);
});

[hugoduraes]

Yes, this is occurring. Just noticed this too

[CacheControl]

@ferrants thanks for giving this a go and providing steps to reproduce. This is actually current behavior of the library, as demo'd in this temp pr #73 based off of master. Issue #50 I think relates to as well.

Essentially allowUndefinedFacts means that the fact ids themselves need to be missing (not existing with a value of 'undefined', as in your example). This is not exactly intuitive, but it's current behavior.

I'm definitely open to changing how this works, I just want to keep this PR focused on the performance element before we introduce new changes. If I've missed something and behavior has changed LMK and I'll re-examine.

[ferrants]

@CacheControl , I appreciate the responsiveness and opening an issue based on this. I upgraded from 2.0.3 and added some more tests and caught it. Validated that this also happens in 2.0.3, so no change in behavior 👍

I'll be running a more performance-based test tomorrow and will post back on the results.

[CacheControl]

Awesome, look forward to the results. I have some time tonight and this alpha candidate has been available for some time, so I'm going to go ahead and merge this + cut a new minor.

[ferrants]

We saw roughly a 50% speed bump, file that was taking 24 hours would have only taken about 16 to 18 hours, which was nicer, but not fast for us, so we had to strip out the library from our batch file processing. I don't think we chose the right use case for this library with batch file processing. We are still using this library in other places.