comunica / sparqlee

⚙️ SPARQL expression evaluator library - Moved to @comunica/expression-evaluator
https://www.npmjs.com/package/sparqlee
14 stars 7 forks source link

Speed up new type system when using no cache #120

Closed jitsedesmet closed 3 years ago

jitsedesmet commented 3 years ago

This PR just removes the use of Object.entries in the getSubTreeWithArg function. This almost halves the execution time of the new type system when not using a cache or when the cache misses.

It can be seen as a part of #119 and might provide an example of the kind of optimizations we need.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.2%) to 89.536% when pulling ee51ae35050a5d5a22b6de5cc454e522b451b004 on jitsedesmet:execution-time/increase-no-cache-speed into 003af86ec98483e3f4e572637282c747174d0fb3 on comunica:master.

jitsedesmet commented 3 years ago

That's a good question. I think it depends on what you find most important. We could removethis.subTrees and just perform a linear search over the new overLoads array. This would only effect initialization time. So, removing the this.subTrees object would:

Should I remove it? I'm thinking yes.

wschella commented 3 years ago

Memory usage also has a performance impact with allocation/deallocation and such. Initialization cost is negligible, there is only one tree per function.

I think the key thing here is: will it make the code more clear and simpler? I feel it will.

rubensworks commented 3 years ago

Alright, so if everything agrees that removing this.subTrees will be a good thing, let's go forward with that plan before merging this PR :-)

wschella commented 3 years ago

I give my :+1:

jitsedesmet commented 3 years ago

I assume this still works in Comunica as well?

Sure does! :)

rubensworks commented 3 years ago

Thanks!