use LazyMap in extension constructors #45
Conversation
| if (code != null) 'code': code, | ||
| }); | ||
| }) : this.fromJson(LazyMap( | ||
| [ |
There was a problem hiding this comment.
Fwiw, I don't love that we will allocate a list still for each call to these. It might be worth trying to some up with something better?
There was a problem hiding this comment.
The code also allocates a closure over this.
The per-object closure could be fixed by making this and explicit argument to the closure, and using a static method.
Calling the closure for each present key is likely to be O(N^2) as the switch-on-string generally compiles to an if-then-else chain on the string comparisons. For small N, no problem, but at some point all the string comparisons will dominate,
There was a problem hiding this comment.
Thanks! I was thinking along these lines yesterday, it was great to find an implementation in my inbox this morning, I spent today adding some benchmarking on top and exploring a third way, writeup here
tl;dr this is already nearly 3x faster than using standard maps; I think we can get rid of the list+closure using extension type builders, and it does give some further speedup, to 3.5x; still some way from proving that it works though, input and suggestions welcome :)
There was a problem hiding this comment.
The code also allocates a closure over
this.
Is that still true given this is a redirecting constructor on an extension type? There is no this in scope.
Calling the closure for each present key is likely to be O(N^2) as the switch-on-string generally compiles to an if-then-else chain on the string comparisons. For small N, no problem, but at some point all the string comparisons will dominate,
For sure, but the number of keys is never going to be very large here, so I think it should be fine.
Thanks! I was thinking along these lines yesterday, it was great to find an implementation in my inbox this morning, I spent today adding some benchmarking on top and exploring a third way, writeup here
Cool, will take a look :)
We don't have a benchmark and might want to wait to land this until we do. Thoughts?
Mostly just wanted to hack on something for the last bit of my day 🤣