cached this.content and this.socreFunction ( faster 7.5%), and have s…

[gamealchemist]

I optimized the Binary Heap in a few ways : • cached the properties that were used several times, content and scoreFunction ( 7.5% faster). • cached scoreElement in sinkDown ( a bit better ). • A few others small things. • BUT most improvement (80% !!! ) came from using -1 and not null as the default value for swap.

The benchmark went from near 2ms • to near 1.8 ms with the first changes, • then to near 1.0 ms with the swap = -1 thing.

[tedivm]

I've found that using this version as a drop in replacement does not work properly. It provides suboptimal paths. I'll poke through at some point to see what optimizations work or not here, but for now I would recommend against including these changes.

[gamealchemist]

My Bad !!! line 391 should read :

 if ( scoreFunction(child2) < (swap < 0 ? elemScore : child1Score)) {

( sorry, not fluent enough with git to push the change.... )

[tedivm]

@gamealchemist: you just need to update the code on your branch and this pull request will automatically update with your new commit.

[tedivm]

You can also just accept the pull request I made against your branch

[gamealchemist]

Thanks for the pool. Is it working now ? ( Faster ? )

[tedivm]

It works now, but I didn't benchmark it so I can't say one way or another about performance.

[bgrins]

There are a lot of unrelated changes in this PR that make it hard for me to review and follow.

BUT most improvement (80% !!! ) came from using -1 and not null as the default value for swap.

Is it possible to have a simpler commit that just does that change? I can't tell what the size/scope of that change is in the diff.

[gamealchemist]

Well, all other changes are about caching ( var content=this.content; mainly ) so there's no threat inside...

[tedivm]

@gamealchemist - just break it up into smaller pull requests. One that does the in function 'caching' and another that uses -1 instead of null.

[gamealchemist]

Again : other changes are just about caching. Any bug would show at once (the cache vars would be undefined -->> an exception thrown ).

[bgrins]

Again : other changes are just about caching. Any bug would show at once (the cache vars would be undefined -->> an exception thrown ).

I'm not really interested in taking any of the caching changes / 'other small things' unless if we can properly benchmark it across engines and it makes a significant improvement. More to the point, it makes it harder to follow exactly what the swap change is doing. Those two things should at least be broken into separate commits since they are separate logical chunks of work.