Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent unnecessary recursive applications of _determineActualTypes #177

Merged
merged 1 commit into from
Dec 12, 2017

Conversation

davidchambers
Copy link
Member

Commit message (modified to take advantage of GitHub and Markdown features):

This commit fixes an error in #78 which resulted in unnecessary recursive applications of _determineActualTypes when determining the types of a collection whose elements are members of a unary type.

For a given collection c whose elements are members of some unary type, the complexity of the faulty algorithm is O(tⁿ), where t is the number of types of which all the inner values of c's elements are members, and n is the number of elements of c.

The actual types of the first element of c should be determined, once, then refined by interrogating the remaining elements of c. The faulty algorithm, though, would determine the actual types of elements of c t¹ + t² + t³ + … + tⁿ times. For example:

c = [[1], [2], [3], [4], [5]]
t = 3 (Number, ValidNumber, FiniteNumber)
n = 5

3^1 + 3^2 + 3^3 + 3^4 + 3^5

The superfluous recursion produced duplicate results, prompting the deduplication added in #159.

This commit prevents superfluous recursion, obviating the need for deduplication. The complexity is now O(tn), a dramatic improvement!

In order to visualize the faulty algorithm one should first apply this patch on master:

--- a/index.js
+++ b/index.js
@@ -918,12 +918,23 @@
     }
   }
 
+  var __sentinel = {};
+  var __lastSeen = __sentinel;
+
   //  _determineActualTypes :: ... -> Array Type
   function _determineActualTypes(
     env,            // :: Array Type
     seen,           // :: Array Object
     values          // :: Array Any
   ) {
+    if (__lastSeen === __sentinel) {
+      process.stdout.write(Z.toString(values) + ' ');
+    } else if (!Z.equals(values, __lastSeen)) {
+      process.stdout.write('\n' + Z.toString(values) + ' ');
+    }
+    process.stdout.write('I');
+    __lastSeen = values;
+
     var recur = _determineActualTypes;
 
     function refine(types, value) {

One can then run the following commands to see how t affects the number of operations:

$ node -e 'const $ = require ("./"); $.create ({checkTypes: true, env: [$.Array ($.Unknown), $.Number]}) ("thunk") ({}) ([$.TypeVariable ("a")]) (() => [[1], [2], [3], [4], [5]]) ()' && echo
[[1], [2], [3], [4], [5]] I
[1] I
[2] I
[3] I
[4] I
[5] I

$ node -e 'const $ = require ("./"); $.create ({checkTypes: true, env: [$.Array ($.Unknown), $.Number, $.ValidNumber]}) ("thunk") ({}) ([$.TypeVariable ("a")]) (() => [[1], [2], [3], [4], [5]]) ()' && echo
[[1], [2], [3], [4], [5]] I
[1] I
[2] II
[3] IIII
[4] IIIIIIII
[5] IIIIIIIIIIIIIIII

$ node -e 'const $ = require ("./"); $.create ({checkTypes: true, env: [$.Array ($.Unknown), $.Number, $.ValidNumber, $.FiniteNumber]}) ("thunk") ({}) ([$.TypeVariable ("a")]) (() => [[1], [2], [3], [4], [5]]) ()' && echo
[[1], [2], [3], [4], [5]] I
[1] I
[2] III
[3] IIIIIIIII
[4] IIIIIIIIIIIIIIIIIIIIIIIIIII
[5] IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII

The results are very different on davidchambers/perf:

$ node -e 'const $ = require ("./"); $.create ({checkTypes: true, env: [$.Array ($.Unknown), $.Number]}) ("thunk") ({}) ([$.TypeVariable ("a")]) (() => [[1], [2], [3], [4], [5]]) ()' && echo
[[1], [2], [3], [4], [5]] I
[1] I

$ node -e 'const $ = require ("./"); $.create ({checkTypes: true, env: [$.Array ($.Unknown), $.Number, $.ValidNumber]}) ("thunk") ({}) ([$.TypeVariable ("a")]) (() => [[1], [2], [3], [4], [5]]) ()' && echo
[[1], [2], [3], [4], [5]] I
[1] I

$ node -e 'const $ = require ("./"); $.create ({checkTypes: true, env: [$.Array ($.Unknown), $.Number, $.ValidNumber, $.FiniteNumber]}) ("thunk") ({}) ([$.TypeVariable ("a")]) (() => [[1], [2], [3], [4], [5]]) ()' && echo
[[1], [2], [3], [4], [5]] I
[1] I

This fix will dramatically reduce the performance impact of type checking, rendering sanctuary-js/sanctuary#464 unnecessary. :)

This commit fixes an error in 2f94dbc which resulted in unnecessary
recursive applications of _determineActualTypes when determining the
types of a collection whose elements are members of a unary type.

For a given collection c whose elements are members of some unary type,
the complexity of the faulty algorithm is O(t^n), where t is the number
of types of which all the inner values of c's elements are members, and
n is the number of elements of c.

The actual types of the first element of c should be determined, once,
then refined by interrogating the remaining elements of c. The faulty
algorithm, though, would determine the actual types of elements of c
t^1 + t^2 + t^3 + ... + t^n times. For example:

    c = [[1], [2], [3], [4], [5]]
    t = 3 (Number, ValidNumber, FiniteNumber)
    n = 5

    3^1 + 3^2 + 3^3 + 3^4 + 3^5

The superfluous recursion produced duplicate results, prompting the
deduplication added in 437f0a4.

This commit prevents superfluous recursion, obviating the need for
deduplication. The complexity is now O(tn), a dramatic improvement!
@gabejohnson
Copy link
Member

These numbers are impressive David!

Have you given any thought to putting some benchmarks in place? It would be nice to avoid performance regressions in the future.

Copy link
Member

@Avaq Avaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@davidchambers
Copy link
Member Author

Have you given any thought to putting some benchmarks in place? It would be nice to avoid performance regressions in the future.

Benchmarks would be very nice indeed! I'll tie up the loose ends that are blocking sanctuary-js/sanctuary-benchmark#1 in the near future. :)

@davidchambers davidchambers merged commit 59f918c into master Dec 12, 2017
@davidchambers davidchambers deleted the davidchambers/perf branch December 12, 2017 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants