Fork me on GitHub
#meander
<
2021-07-07
>
noprompt00:07:06

@huxley Yes. I’m aware of this problem and have been meaning to fix it.

noprompt00:07:52

Also, thank you for bringing it up.

noprompt00:07:03

I would’ve forgotten.

noprompt00:07:14

And then have been back at “meaning to fix it”.

ribelo00:07:41

Unfortunately, I don't think I can help you here

noprompt00:07:22

Not a problem. I wouldn’t want anyone to suffer fixing it anyway because IIRC the way the dissoc is put there is a bit of a hack.

noprompt00:07:27

Killing those will help a lot with the performance of map code.

noprompt00:07:40

Especially for larger maps.

ribelo00:07:46

I look at what can be done to speed things up following the unsafe patch

ribelo00:07:20

With such a simple map, this dissoc takes 40% of the total time

ribelo00:07:58

and all in all I can do very little

noprompt00:07:24

OK. I’ll make this a priority this week.

ribelo00:07:56

there doesn't seem to be an urgent need

noprompt00:07:17

I’m pretty sure the problem is due to meander.match.syntax.epsilon/expand-map. This function tries to rewrite the :map node such that certain keys are prioritized, :as is desugared, etc.

noprompt00:07:08

It should only perform the dissoc when there are rest patterns involved.

noprompt00:07:43

{:a ?x} should not cause a dissoc to be compiled but {:a ?x & ?rest} definitely should.

ribelo00:07:11

it seems not, because after adding the print inside meander.match.syntax.epsilon/expand-map nothing happens :thinking_face:

noprompt00:07:40

Hmm… I should probably start a REPL.

noprompt00:07:03

Ah, you’re right.

ribelo00:07:12

I could be wrong, because the meander is HUGE and it's easy to get lost here

noprompt00:07:52

Line 726 of meander.match.epsilon

rest-rhs (r.ir/op-bind rest_target (r.ir/op-eval `(dissoc ~target [email protected]))
           rest-ir)

noprompt00:07:27

> meander is HUGE I know. I’m not proud of that. zeta will be much, much smaller.

noprompt00:07:17

The current state of the project is the result of scope creep, and experimentation.

ribelo00:07:32

that wasn't meant to sound like an objection

noprompt00:07:40

I know. 🙂

noprompt00:07:28

Seems like I can get rid of the dissoc by changing that line to check if the :rest-map is set.

noprompt00:07:47

OK seems like that worked and tests pass.

noprompt00:07:29

(let [x {:a 1}]
  (m/match x
    {:a ?x, :b ?y, :c ?z}
    ?x))
;; =>
(let*
  [R__12612
   (let*
     [T__12607 x]
     (if (map? T__12607)
       (let* [T__12610 (. T__12607 valAt :c)
              T__12609 (. T__12607 valAt :b)
              T__12608 (. T__12607 valAt :a)]
         (let* [?x T__12608]
           (let* [?y T__12609] (let* [?z T__12610] ?x))))
       meander.match.runtime.epsilon/FAIL))]
  (if (meander.match.runtime.epsilon/fail? R__12612)
    (throw (ex-info "non exhaustive pattern match" {}))
    R__12612))
No dissoc.

noprompt00:07:38

OK fixed on epsilon. I’ll cut a release soon.

noprompt00:07:47

That should also include the new & abilities.

noprompt00:07:32

I’ve gotta get up and get ready for dinner, etc.

ribelo00:07:36

I added PR

ribelo00:07:36

It's 3 a.m. at my place, so I'm off. 😜

noprompt00:07:54

OK! A quick glance at this and it looks good.

noprompt00:07:33

I’m guessing fast means “Don’t check anything and call native methods?” 🙂

ribelo00:07:17

When I get settled in, maybe I'll think of something else

noprompt00:07:42

I’m thinking maybe this is what unsafe should be.

ribelo00:07:31

I'm trying to catch up with datascript speed with my naive implementation of datalog using meander

noprompt00:07:07

Yeah, lemme think about it for a moment. I doubt it would break anyone to make this is the implementation of unsafe.

noprompt00:07:16

It’s new stuff anyway.

ribelo00:07:50

I think so too, but I saw that you changed from valAt to get

noprompt00:07:35

I did because I was under the impression that using get without type checks was “fast enough”.

ribelo00:07:49

and from what I've been testing, .get is probably faster than .valAt

noprompt00:07:10

Ah, interesting.

noprompt00:07:14

Works for me.

ribelo00:07:25

there are cases where unsafe is slower than safe

noprompt00:07:15

TBH I think unsafe, at a minimum, should be as fast safe.

ribelo00:07:49

in case of a large map and many lookups, the benefit of not checking if the object is a map is negated due that the get is much slower

👍 3
ribelo00:07:57

Go to dinner, I'm going to bed. I have to get up in 4 hours:sweat_smile:

noprompt18:07:25

@jimmy do you foresee/have any objections to having unsafe do what the fast flag is doing? IOW should the patch @huxley be the implementation of unsafe?

noprompt18:07:31

Because if skipping the type checks and using get is slower than having type checks and using .valAt, etc. it doesn’t really make a lot of sense for the unsafe option, as it is implemented now, to exist.

jimmy19:07:22

I don't think so. Though I personally think depending on ILookup (.valAt) instead of APersistentMap (.get) gives us more flexibility. We don't want to only work on APersistentMaps for that. Anything implementing ILookup should be allowed (in my view). Maybe for non-unsafe where we know map? is true we could use .get if it is truly faster. For example, one thing we might want to support in unsafe mode is transients, but transients are not APersistentMaps.

noprompt19:07:48

+-------------+----------------+----------------------------+
| type-check? | native-method? | result                     |
+-------------+----------------+----------------------------+
| true        | false          | (f object ,,,)             | < Slow
+-------------+----------------+----------------------------+
| true        | true           | (.method ^Type object ,,,) | < Safe (Default)
+-------------+----------------+----------------------------+
| false       | false          | (f object ,,,)             | < Unsafe
+-------------+----------------+----------------------------+
| false       | true           | (.method ^Type object ,,,) | < Dangerous
+-------------+----------------+----------------------------+

noprompt19:07:16

> Maybe for non-unsafe where we know map? is true we could use .get if it is truly faster.

noprompt19:07:23

I think thats a wise response.

noprompt19:07:17

The table up there captures the general picture of safetiness.

noprompt19:07:49

Instead of fast I think we should dub the flag “dangerous”.

2
noprompt19:07:23

Note, that I’m throwing bounds-check? into the same basket as type-check? since it has a similar effect.

markaddleman19:07:55

I recently updated to meander 0.0.626 and I get the following warnings when I start the REPL:

Performance warning, meander/util/epsilon.cljc:123:3 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:157:3 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:191:3 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:221:3 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:314:6 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:291:4 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:374:6 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:348:4 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:435:3 - case has int tests, but tested expression is not primitive.
Performance warning, meander/util/epsilon.cljc:630:3 - case has int tests, but tested expression is not primitive.
Reflection warning, meander/util/epsilon.cljc:714:24 - reference to field val can't be resolved.
I doubt it's a problem but wanted to call it out just in case

noprompt19:07:54

@markaddleman I recently fixed those errors and all but the last of those should go away with the next release.

👍 2
noprompt19:07:51

I’m hoping that we come to an agreement/patch on some of the new flag stuff and I can release that in addition to the update to & patterns.

noprompt19:07:39

BTW in case anyone is interested, the algorithm I used to generate the space for map patterns like

{&1 ?m1 &2 ?m2 &3 ?m3}
came from the Art of Computer Programming Volume 4A on the topic of partitions.

notbad 2
noprompt19:07:37

I’m sure there’s some more good stuff in there. Its a big book!

markaddleman19:07:04

On a related topic, I just discovered a pattern that compiled in 0.0.602 but does not compile in 626:

(comment
  ; does not compile
  (m/rewrites {}
    {:group-defaults (m/scan {:attribute       {:alias (m/some ?alias)}
                              :from-expression (m/some ?expression)})
     :group-by       (m/scan {:alias ?alias :as ?group-by})}
    {:group-by [{:default {:from-expression ?expression} & ?group-by}]}

    {:group-by       (m/scan {(m/some :alias) ?a :as ?group-by})
     :group-defaults (m/scan {:attribute {:alias (m/not ?a)}})}
    {:group-by [?group-by]}))

markaddleman19:07:28

Meander complains Unable to resolve symbol: ?a in this context

markaddleman19:07:52

Oddly, this pattern compiles on .626:

(m/rewrites {}
    {:group-by       (m/scan {(m/some :alias) ?a :as ?group-by})
     :group-defaults (m/scan {:attribute {:alias (m/not ?a)}})}
    {:group-by [?group-by]})

markaddleman19:07:35

Both patterns compile successfully on 602

noprompt19:07:56

I’ll take a look.

🙏 2
markaddleman20:07:36

whoopsie: My previous version of meander was 602 (not 606). Edited previous messages to make that clear

noprompt20:07:20

OK. I’ll compare the tags and see if I can narrow down the problem.

ribelo22:07:28

depending on how we test, .get is faster than .valAt about 5-10%, I don't know if it's worth worrying about

ribelo22:07:34

but the situation looks quite different for cljs

ribelo22:07:35

(let [m {:a 1}]
  (enc/qb 1e6
    (get m :a)
    (.get m :a)
    (m :a)))
;; => [90 26 42] in ms