Speed up literal transformation

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 literal transformation #121

Closed jitsedesmet closed 3 years ago

jitsedesmet commented 3 years ago

This PR builds on top of #120 . It increases the execution time to a point where we almost want to keep the new type system I think.

Execution times:

bench addition no overloadCache x 17.26 ops/sec ±2.23% (47 runs sampled)
bench addition with overloadCache x 26.19 ops/sec ±1.31% (46 runs sampled)
bench addition non experimental x 26.96 ops/sec ±1.34% (48 runs sampled)
Mean execution time without cache 0.05794374734042553
Mean execution time with cache 0.03818295654347826
Mean execution time with nonExperimental 0.037093097125
Fastest is bench addition non experimental

This means there is only a +-5% decrease when using the new type system.

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 remained the same at 89.536% when pulling 4e732bf922b8e0e6fb3a6e96916c77934962141d on jitsedesmet:execution-time/increase-transformLiteral-execution-time into f3b93b897aa2b416bf100a5c75e6b163752aa91a on comunica:master.

wschella commented 3 years ago

Should we rebase the relevant commits onto the master for this PR and the other similar ones?

jitsedesmet commented 3 years ago

Yes, we should :) I plan on doing this before Monday but a lot is happening right now so it might get delayed. This PR and #120 should just get rebased and then they should be ready to merge. Like I said, we should first finish #120 and Ruben asked a question there that needs to be looked into.

rubensworks commented 3 years ago

Since this depends on #120, let's put this one on hold until the changes discussed in #120 are applied.

jitsedesmet commented 3 years ago

@rubensworks I will now just merge this with #120 and resolve the conflicts so we can merge this into master?

rubensworks commented 3 years ago

Sounds good!

jitsedesmet commented 3 years ago

Something weird happened during the merge, but it should be fine now.

wschella commented 3 years ago

(Will wait again for merging until @wschella gives his +1)

I'm very busy this week and coming ones, so I don't really have the bandwidth to cover this PR together with the related ones in depth. After a quick glance everything seems definitely okay.

Around December I will give a pass to the new system in it's entirety myself. But everything can definitely be merged.

rubensworks commented 3 years ago

@wschella No worries, a quick glance is fine :-)

rubensworks commented 3 years ago

Great work!