Skip to content

feat: minibench -> add #178

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

Merged
merged 4 commits into from
Dec 26, 2020
Merged

feat: minibench -> add #178

merged 4 commits into from
Dec 26, 2020

Conversation

srghma
Copy link
Contributor

@srghma srghma commented Aug 26, 2020

No description provided.

@hdgarrood
Copy link
Contributor

While I think you are probably right in that this will be faster, I would like to see benchmarks of this version versus the previous one before approving.

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

@hdgarrood how would you suggest approaching this? the benchmarks are not implemented in the repo

@natefaubion
Copy link

PureScript minibench is in core https://github.com/purescript/purescript-minibench, but I don't know if it already has a transitive dependency on arrays.

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

before

var mapMaybe = function (f) {
    return concatMap((function () {
        var $94 = Data_Maybe.maybe([  ])(singleton);
        return function ($95) {
            return $94(f($95));
        };
    })());
};
Array
===
mapMaybe
---------------
mapMaybe (101)
mean   = 10.51 μs
stddev = 40.30 μs
min    = 3.56 μs
max    = 695.12 μs
mapMaybe (10001)
mean   = 295.67 μs
stddev = 69.77 μs
min    = 249.30 μs
max    = 545.49 μs

after

```js
var mapMaybe = function (f) {
    return function (input) {
        return (function __do() {
            var output = Data_Array_ST.empty();
            var inputIterator = Data_Array_ST_Iterator.iterator(function (v) {
                return index(input)(v);
            })();
            Data_Array_ST_Iterator.iterate(inputIterator)(function (inputElem) {
                var v = f(inputElem);
                if (v instanceof Data_Maybe.Nothing) {
                    return Control_Applicative.pure(Control_Monad_ST_Internal.applicativeST)(Data_Unit.unit);
                };
                if (v instanceof Data_Maybe.Just) {
                    return Data_Functor["void"](Control_Monad_ST_Internal.functorST)(Data_Array_ST.push(v.value0)(output));
                };
                throw new Error("Failed pattern match at Data.Array (line 631, column 7 - line 633, column 64): " + [ v.constructor.name ]);
            })();
            return Data_Array_ST.unsafeFreeze(output)();
        })();
    };
};
Array
===
mapMaybe
---------------
mapMaybe (101)
mean   = 43.53 μs
stddev = 110.03 μs
min    = 14.30 μs
max    = 1.70 ms
mapMaybe (10001)
mean   = 1.62 ms
stddev = 213.01 μs
min    = 1.38 ms
max    = 2.90 ms

O_O

@natefaubion
Copy link

You should verify that you are running node with --expose-gc.

@hdgarrood
Copy link
Contributor

Huh!

@natefaubion
Copy link

I think the thing you have to remember about ST is that even with magic-do, you are going to be allocating lots of Effect closures, which can be significantly more than whatever immutable data you are allocating (this depends greatly on the algorithm though). ST needs STFn types to get around this limitation.

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

with --expose-gc

before

Array
===
mapMaybe
---------------
mapMaybe (101)
mean   = 14.63 μs
stddev = 119.57 μs
min    = 3.58 μs
max    = 3.56 ms
mapMaybe (10001)
mean   = 405.78 μs
stddev = 767.98 μs
min    = 248.40 μs
max    = 7.14 ms

after

Array
===
mapMaybe
---------------
mapMaybe (101)
mean   = 42.57 μs
stddev = 105.71 μs
min    = 15.01 μs
max    = 1.59 ms
mapMaybe (10001)
mean   = 1.94 ms
stddev = 2.49 ms
min    = 1.41 ms
max    = 22.38 ms

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

ST needs STFn types to get around this limitation

which are not implemented, right?


I'll try with ffi

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

this one 2x better

Array
===
mapMaybe
---------------
mapMaybe (101)
mean   = 8.44 μs
stddev = 33.12 μs
min    = 1.68 μs
max    = 619.76 μs
mapMaybe (10001)
mean   = 220.75 μs
stddev = 765.45 μs
min    = 90.65 μs
max    = 7.13 ms

@hdgarrood
Copy link
Contributor

Unfortunately we can't depend on nullable here, as core libraries may only depend on other core libraries.

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

@hdgarrood what to do?

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

how about this

Array
===
mapMaybe
---------------
mapMaybe (101)
mean   = 5.41 μs
stddev = 24.38 μs
min    = 1.14 μs
max    = 498.88 μs
mapMaybe (10001)
mean   = 164.07 μs
stddev = 642.34 μs
min    = 46.47 μs
max    = 5.76 ms

@hdgarrood
Copy link
Contributor

Actually I think the nullable dependency is probably less of an issue than the implementation you've gone with here, specifically the fact that it's completely FFI-based. We should really be moving in the direction of using less FFI in this code, as using the FFI for things like this causes pain for alternate backends (as they would then need to implement mapMaybe in their FFI too if we shipped this). Using FFI for this sort of thing also means that we miss the opportunity to develop the building blocks which would permit us to write fast code in PureScript. I think the answer here may be an STFn type with compiler support.

@hdgarrood
Copy link
Contributor

We definitely shouldn't depend on the .value0 representation here, in any case.

@srghma
Copy link
Contributor Author

srghma commented Aug 26, 2020

@hdgarrood where can I find issue about adding STFn?

@hdgarrood
Copy link
Contributor

I don't think we have an issue for it - there isn't much discussion of it so far. The idea would be that we have a collection of data types like

STFn1 :: Type -> Type
STFn2 :: Type -> Type -> Type
STFn3 :: Type -> Type -> Type -> Type

and so on, like EffectFn{1,2,3}, which would perform their effects immediately rather than returning a nullary function, together with inlinable functions like

runSTFn1 :: forall a h r. STFn1 a r -> a -> ST h r
runSTFn2 :: forall a b h r. STFn2 a b r -> a -> b -> ST h r

and so on, so that code like

ST.run do
  arr <- STArray.new
  _ <- runSTFn2 STArray.push arr 3
  pure (STArray.unsafeFreeze arr)

might compile to something like

function () {
  var arr = STArray.new();
  Data_Array_ST.push(arr, 3);
  return unsafeFreezeImpl(arr);
}

where Data_Array_ST.push would have the type forall a. STFn2 (Array a) a Int.

@srghma srghma changed the title refactor: mapMaybe -> improve speed feat: minibench -> add Aug 27, 2020
@srghma
Copy link
Contributor Author

srghma commented Aug 27, 2020

@hdgarrood done

@hdgarrood
Copy link
Contributor

I'll approve once these last two points are addressed - thanks!

@srghma
Copy link
Contributor Author

srghma commented Oct 24, 2020

@hdgarrood everything fixed

@JordanMartinez
Copy link
Contributor

I've fixed the merge conflicts by updating dependencies to master and updating psa to v0.8.0

@JordanMartinez
Copy link
Contributor

Is this ready to merge then?

@JordanMartinez
Copy link
Contributor

Ping

@hdgarrood hdgarrood merged commit a3909e0 into purescript:master Dec 26, 2020
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.

5 participants