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

Frog: compareTo can be made much faster #423

Closed
rakudrama opened this issue Nov 11, 2011 · 4 comments
Closed

Frog: compareTo can be made much faster #423

rakudrama opened this issue Nov 11, 2011 · 4 comments
Labels
P3 A lower priority bug or feature request

Comments

@rakudrama
Copy link
Member

The attached test uses a.compareTo(b)

sh.dart is a Skew Heap.
sh_test.dart uses the heap.

time dart_bin sh_test.dart
real 0m0.145s
user 0m0.140s
sys 0m0.000s

time frogsh sh_test.dart (includes <1s compile time)
real 0m3.851s
user 0m3.810s
sys 0m0.020s

The problem appears to be that a.compareTo(b) boxes the number a.
It would be better to avoid the boxing, similar to how $add(x, y) is called to avoid boxing on non-resolved operator+.

I hacked output of Frog, changing the call a.compareTo(b) to $compareTo(a, b)

function $compareTo(a, b) {
  if (typeof a == 'number' && typeof b == 'number') {
    if (a < b) return -1;
    if (a > b) return 1;
    if (a != 0) return 0;
    var signA = 1/a;
    var signB = 1/b;
    if (signA < signB) return -1;
    if (signB < signA) return 1;
    return 0;
  }
  return a.compareTo(b);
}

The time changed to ~0.400s, i.e 10x faster.

A 'production quality' $compareTo should also special case string vs string.

Similar hacking on a program that uses List.sort((a, b) => a.compareTo(b)) yielded a 40x speedup.


Attachments:
sh.dart (1001 Bytes)
sh_test.dart (1.43 KB)

@jmesserly
Copy link

IMO we should fix this for all native functions on String and num, with a uniform solution. If done correctly, it can even make a lot of the code cleaner. Most of the stubs in corejs.dart can go away, and instead become "native" methods on num.dart, String.dart again.

@DartBot
Copy link

DartBot commented Nov 17, 2011

This comment was originally written by jimhug@google.com


SGTM!

@DartBot
Copy link

DartBot commented Nov 22, 2011

This comment was originally written by jimhug@google.com


Marking as low pri as this would be okay to let slip until next quarter. Please edit the priority if you think this is critical to using frog in the near term.


Removed Priority-Medium label.
Added Priority-Low label.

@jmesserly
Copy link

Fixed by switching to strict mode. Depending on what our browser target set is, we might need to add $compareTo, but this should leave us in good place for now.


Added Fixed label.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

3 participants