Feature/optimize #39

Merged
merged 3 commits into from Jun 14, 2013

Projects

None yet

2 participants

@mikepb
Contributor
mikepb commented Dec 29, 2012

Use depth count instead of array lookup to shave ~400ms from pack using project benchmark.

@godsflaw
Collaborator

Are you doing the 512 depth check as a heuristic for cycle detection? I ask because I only had time for a quick read of the code. I know the old class was trying to detect cycles, even though that code was sub-optimal for multiple reasons. There were some better methods O(n*log(n)) suggested in the check method comments. If I recall correctly, this cycle detection method may have even causes false positives.

That being said, if you are using 512 as a heuristic for cycle detection, are we still worried about cycle detection? It's hard to argue with O(1), unless this just blows up on people's real data because there are cases where this throws an error. Again, quick review.

My boat is offloading, I will check in on this later. Let me know if my reading of your patch was off.

@mikepb
Contributor
mikepb commented Dec 30, 2012

I agree with you that it is a heuristic that could "blow up" on people's data. I consider cycles to be a programmer error that should not be in production code. I used a max depth of 512 only because it was used in the JS version of the library. The program would probably choke without tail recursion optimization anyway...

@godsflaw godsflaw merged commit 19f37c0 into pgriess:master Jun 14, 2013
@godsflaw
Collaborator

I merged this manually, but have not version bumped yet. I want to resolve a few more issues first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment