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

Move allocation out of _Utils_compare (improving performance) #918

Closed
wants to merge 1 commit into
base: dev
from

Conversation

Projects
None yet
4 participants
@Skinney
Contributor

Skinney commented Nov 1, 2017

In my work on a new Dict implementation, I discovered that one of the functions being called the most is _Utils_compare. By moving the object allocation out of the function (pre-allocating) we can improve performance of the function.

In my benchmark, Dict.get goes from a mean runtime of ~30ns per call (for a Dict of 100 key-value pairs) to ~26ns. If my math is correct (it rarely is) that is a 13% performance increase.

I also tried the following, but it did not offer better performance and is more code:

var lt = { $: 'LT' };
var eq = { $: 'EQ' };
var gt = { $: 'GT' };

var _Utils_compare = F2(function(x, y)
{
    var ord = _Utils_cmp(x, y);
    return ord < 0 ? lt : ord > 0 ? gt : eq;
});

(The actual benchmarks were performed by modifying a 0.18 compiled output by hand. Even though the code looks slightly different in this 0.19-branch, I don't see why the results would be any different).

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Nov 1, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Nov 1, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Nov 1, 2017

Member

I think it would be best to refer to elm_lang$core$Basics$LT such that it gets minified better. This also means the runtime representation can be chosen by the compiler more easily.

I'll keep this open as a reminder of this change, but there are a few unknown things that make it hard to make a change at this exact moment.

Member

evancz commented Nov 1, 2017

I think it would be best to refer to elm_lang$core$Basics$LT such that it gets minified better. This also means the runtime representation can be chosen by the compiler more easily.

I'll keep this open as a reminder of this change, but there are a few unknown things that make it hard to make a change at this exact moment.

@justinmimbs

This comment has been minimized.

Show comment
Hide comment
@justinmimbs

justinmimbs Nov 2, 2017

Regarding the idea of using primitives at runtime to represent argument-less union types: I hand-modified the JS output of Robin's Dict.LLRB.get function to use cmp instead of compare, and it saw a 77% increase in ops/sec in Chrome (using dicts of 100 elements with ints as keys).

i.e. from

var _p16 = A2(_elm_lang$core$Basics$compare, targetKey, _p15._1);
switch (_p16.ctor) {
    case 'LT':

to

var _p16 = _elm_lang$core$Native_Utils.cmp(targetKey, _p15._1);
switch (_p16) {
    case -1:

justinmimbs commented Nov 2, 2017

Regarding the idea of using primitives at runtime to represent argument-less union types: I hand-modified the JS output of Robin's Dict.LLRB.get function to use cmp instead of compare, and it saw a 77% increase in ops/sec in Chrome (using dicts of 100 elements with ints as keys).

i.e. from

var _p16 = A2(_elm_lang$core$Basics$compare, targetKey, _p15._1);
switch (_p16.ctor) {
    case 'LT':

to

var _p16 = _elm_lang$core$Native_Utils.cmp(targetKey, _p15._1);
switch (_p16) {
    case -1:
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 7, 2018

Member

The final version looks like this: https://github.com/elm-lang/core/blob/master/src/Elm/Kernel/Utils.js#L127-L131

There are bunches of inlining tricks we can do to avoid allocations, but the compiler infrastructure is not sophisticated enough to do them right now. So it's not that I don't think of them; it's that they cannot be done without a great deal of infrastructure work to get type information on all expressions. That is something I have my eye on as well.

Member

evancz commented Mar 7, 2018

The final version looks like this: https://github.com/elm-lang/core/blob/master/src/Elm/Kernel/Utils.js#L127-L131

There are bunches of inlining tricks we can do to avoid allocations, but the compiler infrastructure is not sophisticated enough to do them right now. So it's not that I don't think of them; it's that they cannot be done without a great deal of infrastructure work to get type information on all expressions. That is something I have my eye on as well.

@evancz evancz closed this Mar 7, 2018

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